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. Every time we changed base, we'd need to take extra care to make sure base is rolled properly. 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".) > 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. 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. > 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 --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
