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 <[email protected]> wrote: > > > On Wed, Oct 14, 2009 at 3:59 PM, Jeremy Orlow <[email protected]> wrote: > >> On Wed, Oct 14, 2009 at 2:48 PM, Darin Fisher <[email protected]> wrote: >> >>> On Wed, Oct 14, 2009 at 2:47 PM, Darin Fisher <[email protected]>wrote: >>> >>>> On Wed, Oct 14, 2009 at 2:30 PM, Jeremy Orlow <[email protected]>wrote: >>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Darin Fisher <[email protected]>wrote: >>>>> >>>>>> On Wed, Oct 14, 2009 at 2:08 PM, Jeremy Orlow <[email protected]>wrote: >>>>>> >>>>>>> On Wed, Oct 14, 2009 at 2:00 PM, Michael Nordman < >>>>>>> [email protected]> 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 >>>>>>> <[email protected]>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: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
