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. > 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. > > 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. 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 > Personally, I think we should have more functions for converting to base:: > types rather than less. > > > >> >> -Darin >> >> >>> >>>>> Does this sound like a good plan? >>>>> >>>>> J >>>>> >>>> >>>> >>> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
