To be clear: I only weakly advocate Chrome having a SecurityOrigin. I'm also OKwith just using std::string. I think either is better than using GURL.
-Darin On Wed, Oct 14, 2009 at 4:45 PM, Darin Fisher <da...@chromium.org> wrote: > Hmm... it seems useful as a means of making the code more self-documenting > anda bit safer. I'd rather not have people passing strings for origins > since they might be > tempted to parse the strings. > > The momentary translation to strings on the boundary of the WebKit API does > not > completely make this fragile. If the end-points in Chrome use > SecurityOrigin and > the end-points in the WebKit API use WebSecurityOrigin, then developers > will be > naturally inclined to convert between SecurityOrigin and WebSecurityOrigin, > ignoring the toString() getter on WebSecurityOrigin. > > This is a case where existing code should help guide people in the right > direction. > > -Darin > > > > On Wed, Oct 14, 2009 at 4:39 PM, Jeremy Orlow <jor...@chromium.org> wrote: > >> If this is the case, then I don't think a SecurityOrigin wrapper buys us >> anything. Never mind. >> >> >> On Wed, Oct 14, 2009 at 4:37 PM, Darin Fisher <da...@chromium.org> wrote: >> >>> >>> >>> On Wed, Oct 14, 2009 at 3:59 PM, Jeremy Orlow <jor...@chromium.org>wrote: >>> >>>> On Wed, Oct 14, 2009 at 2:48 PM, Darin Fisher <da...@chromium.org>wrote: >>>> >>>>> On Wed, Oct 14, 2009 at 2:47 PM, Darin Fisher <da...@chromium.org>wrote: >>>>> >>>>>> On Wed, Oct 14, 2009 at 2:30 PM, Jeremy Orlow <jor...@chromium.org>wrote: >>>>>> >>>>>>> On Wed, Oct 14, 2009 at 2:23 PM, Darin Fisher <da...@chromium.org>wrote: >>>>>>> >>>>>>>> On Wed, Oct 14, 2009 at 2:08 PM, Jeremy Orlow >>>>>>>> <jor...@chromium.org>wrote: >>>>>>>> >>>>>>>>> On Wed, Oct 14, 2009 at 2:00 PM, Michael Nordman < >>>>>>>>> micha...@google.com> wrote: >>>>>>>>> >>>>>>>>>> +1 SecurityOrigin class >>>>>>>>>> Sounds like a reasonable plan. >>>>>>>>>> I suspect there may already be cases where we're actually >>>>>>>>>> comparing a chrome generated security origin, as produced by >>>>>>>>>> GURL.GetOrigin(), with a webkit generated security origin, as >>>>>>>>>> produced by >>>>>>>>>> WebSecurityOrigin.toString(). So we may want to accelerate the part >>>>>>>>>> of the >>>>>>>>>> plan to do more than opaquely pass around and test webkit generated >>>>>>>>>> representations. >>>>>>>>>> >>>>>>>>>> Also, I think dumi has a use case to crack it open in order to >>>>>>>>>> form file path elements of the form 'scheme_host_port' >>>>>>>>>> >>>>>>>>> >>>>>>>>> Actually, Dumi's case is slightly different. He wants to get >>>>>>>>> SecurityOrigin::databaseIdentifier, right? Maybe WebSecurityOrigin >>>>>>>>> should >>>>>>>>> have a databaseIdentifier() method that outputs a FilePath object? >>>>>>>>> >>>>>>>> >>>>>>>> Dumi has such a method in a CL that he is working on at the moment. >>>>>>>> Also, note: we don't have a way to use FilePath from the WebKit API, >>>>>>>> and >>>>>>>> I'm not sure that we should. We use WebString for file paths in the >>>>>>>> WebKit >>>>>>>> API. >>>>>>>> >>>>>>> >>>>>>> So then he's adding such a method to WebSecurityOrigin that returns a >>>>>>> string? If so, sounds good. What's the CL, btw? >>>>>>> >>>>>> >>>>>> Yes: >>>>>> http://codereview.chromium.org/256073/diff/11001/11029 >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> ... and why not use strings? >>>>>>>>>> * does the string contain a trailing slash, or not? >>>>>>>>>> * in the default port case, does the string contain the default >>>>>>>>>> port number or not? >>>>>>>>>> >>>>>>>>> >>>>>>>>> WebCore::SecurityOrigin handles these for us. I'll make it >>>>>>>>> difficult for a base::SecurityOrigin to be constructed any way >>>>>>>>> besides it >>>>>>>>> coming from WebKit::WebSecurityOrigin (which only comes from >>>>>>>>> WebCore::WebSecurityOrigin). We can then deal with these details only >>>>>>>>> if/when we need to. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Oct 14, 2009 at 1:36 PM, Jeremy Orlow <jor...@chromium.org >>>>>>>>>> > wrote: >>>>>>>>>> >>>>>>>>>>> Right now, we don't have a good story for what to do with >>>>>>>>>>> WebCore::SecurityOrigins in Chromium. We now have a >>>>>>>>>>> WebSecurityOrigin in >>>>>>>>>>> WebKit, but if you want to move the data between processes, you >>>>>>>>>>> need to >>>>>>>>>>> convert it to a string and then send that. In some cases we then >>>>>>>>>>> convert >>>>>>>>>>> the string to a GURL, but this seems like the wrong thing to do >>>>>>>>>>> (more on >>>>>>>>>>> this in a sec). >>>>>>>>>>> To me, the right answer is to create a type in base called >>>>>>>>>>> SecurityOrigin that wraps a string and does equality checks. The >>>>>>>>>>> equality >>>>>>>>>>> checks can be done as string comparisons since the >>>>>>>>>>> WebCore::SecurityOrigin::toString() method canonicalizes it. If, >>>>>>>>>>> in the >>>>>>>>>>> future, we need to do anything more with SecurityOrigins (besides >>>>>>>>>>> transporting them, testing equality, and using them in sets/maps) >>>>>>>>>>> then we >>>>>>>>>>> can make the class more complex. >>>>>>>>>>> >>>>>>>>>>> Why not use GURL? For one, the SecurityOrigin has a "null" state >>>>>>>>>>> which is significant and which isn't represented in GURL. In >>>>>>>>>>> addition, >>>>>>>>>>> there's a lot of operations you can do with a GURL which don't >>>>>>>>>>> really make >>>>>>>>>>> sense in the context of a SecurityOrigin. Passing around a >>>>>>>>>>> SecurityOrigin >>>>>>>>>>> object is also much more self-documenting. But, the fact that GURL >>>>>>>>>>> looks >>>>>>>>>>> like a tempting way to store a SecurityOrigin is actually one of >>>>>>>>>>> the biggest >>>>>>>>>>> reasons why I think we should make a dedicated type. >>>>>>>>>>> >>>>>>>>>>> If people agree with this, my plan is to create such a type in >>>>>>>>>>> base and modify WebKit::WebSecurityOrigin to do conversions to >>>>>>>>>>> base::SecurityOrigin. I'll then convert everything over (or ask >>>>>>>>>>> people to >>>>>>>>>>> do the conversion if it looks scary). Finally, I'll remove >>>>>>>>>>> WebSecurityOrigin::toString(). >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>>> As I mentioned in person, I'm not happy having WebKit API depend on >>>>>>>> base for too many things. I would prefer to not introduce this >>>>>>>> dependency >>>>>>>> since it is a circular dependency (in the way the respective >>>>>>>> repositories >>>>>>>> relate). Circular dependencies are evil. We have them for string16 >>>>>>>> and >>>>>>>> NullableString16. Let's not add more. >>>>>>>> >>>>>>> >>>>>>> If we have one circular dependency on base, why not add more? >>>>>>> >>>>>> >>>>>> Because they can be a source of great pain. This is a slippery slope. >>>>>> We can basically never change base/string16.h or >>>>>> base/nullable_string16.h. >>>>>> I don't want more of that. Things like ChromiumBridge exist so we can >>>>>> avoid having more of these. >>>>>> >>>>> >>>> If we're making a DLL out of WebKit, then you're right. >>>> >>> >>> We are (for debug builds). That has always been the plan :-) >>> >>> But this is not the reason. >>> >>> >>> >>>> Every time we changed base, we'd need to take extra care to make sure >>>> base is rolled properly. >>>> >>> >>> What does rolling base mean? base is part of chromium. chromium depends >>> on webkit. See what I mean? base is not a separately versioned module. >>> >>> >>> >>>> How are we going to provide developers such a DLL, though? If it's >>>> checked in, then whenever someone changes base they can check in a new copy >>>> of WebKit.dll. And, if we do things some other way, I'm sure we can find >>>> other reasonable solutions. >>>> >>>> >>>>> Anyhow, they're already #if'ed. So if someone wanted to use the API >>>>>>> without base, they easily could change that #else to a #elif, right? >>>>>>> Maybe >>>>>>> we should just do that now? >>>>>>> >>>>>> >>>>>> Right, that was my intent. They are currently defined when >>>>>> WEBKIT_IMPLEMENTATION is not defined, but we should probably make >>>>>> consumers >>>>>> opt-in by defining something explicit. >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> I agree the circular dependencies are bad, but they're already there. >>>>>>> And, honestly, the WebKit API (in its current form) is not useful >>>>>>> unless >>>>>>> you are including base. >>>>>>> >>>>>> >>>>>> I disagree. >>>>>> >>>>> >>>> How would you use the WebKit API without base today? The only way is if >>>> you added #elif's to convert to some other set of primitives for URLs and >>>> strings, right? (That's why I qualified my statement with "in its current >>>> form".) >>>> >>> >>> android has no interest in using STL but this webkit api could be very >>> useful to them and other webkit consumers. the #else will change to #elif, >>> i'm sure, in order to support such consumers. >>> >>> >>> >>>> >>>> >>>>> Whatsmore, if we output stuff as a string, then we're just hoping >>>>>>> someone goes ahead and immediately converts that to a SecurityOrigin. >>>>>>> But >>>>>>> there's no guarantee they won't. Or that they won't do >>>>>>> something incredibly stupid before such a conversion. By forcing >>>>>>> things to >>>>>>> go straight to a base::SecurityOrigin (and comments in that code >>>>>>> explaining >>>>>>> why) it'll be much harder for someone to do something dangerous without >>>>>>> doing it willfully. >>>>>>> >>>>>> >>>>>> >>>>>> I think we should just do this: >>>>>> >>>>>> class SecurityOrigin { >>>>>> public: >>>>>> SecurityOrigin(const WebKit::WebSecurityOrigin& origin) : >>>>>> value_(origin.toString()) { >>>>>> } >>>>>> ... >>>>>> private: >>>>>> string16 value_; >>>>>> }; >>>>>> >>>>>> This way there is no way to construct a SecurityOrigin from a string. >>>>>> >>>>> >>>> There's nothing to make you (or even hint that you should) convert it >>>> from a string to a SecurityOrigin and then from a SecurityOrigin to a >>>> string >>>> before turning it into a WebSecurityOrigin. >>>> >>> >>> There is only a default constructor for WebSecurityOrigin. So far we >>> haven't needed to initialize a WebSecurityOrigin in chromium. WebCore does >>> that for us. >>> >>> >>> >>>> In addition, you're going to be incurring 2 extra string >>>> copies every time. I think both of these are pretty compelling reasons >>>> against forcing everyone to convert things to strings and then to the type >>>> it should have been all along. I really don't think this will increase >>>> the maintenance burden _that_ much. >>>> >>> >>> The conversion cost seems minor. There would be a conversion cost to >>> SecurityOrigin no matter what since its members are not going to be >>> WebString. >>> >>> >>> >>>> >>>> >>>>> I think it solves >>>>>> most of the problems you are concerned with. Next, we just need to >>>>>> encourage people >>>>>> to use SecurityOrigin in Chrome whenever they need to refer to a >>>>>> security origin. >>>>>> >>>>>> Also, I think SecurityOrigin should be defined in webkit/glue since >>>>>> base/ should not >>>>>> depend on WebKit API, and SecurityOrigin is really too high level of a >>>>>> concept for >>>>>> base/. >>>>>> >>>>>> -Darin >>>>>> >>>>>> >>>>> Now that I think about it, I think we should change NullableString16 to >>>>> work this way too. >>>>> The only reason we have string16 in WebKit API is because we cannot add >>>>> ctors to >>>>> string16. >>>>> >>>> >>>> I've created a bug for this: >>>> http://code.google.com/p/chromium/issues/detail?id=24844 >>>> >>> >>> Thanks, >>> -Darin >>> >> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
