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. > > > >> 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 > > 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. -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: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
