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 -~----------~----~----~----~------~----~------~--~---
