On Wed, Oct 14, 2009 at 4:51 PM, Jeremy Orlow <jor...@chromium.org> wrote:
> You mean string16, right? I see instances of std::string and string16. I would be happiest if we unified on one. string16 is probably the path of least resistance. std::string has the benefit of being more compact, which for something like this which is really just a bag of bytes is probably a good thing. > > I really don't think it buys us much if it's purely optional that people > put the security origin (in string representation) into a wrapper that then > blocks them from doing anything silly with it. > we could give it a ToString() method. i think the point of SecurityOrigin would be to guide people in the right direction. -darin > > More to the point, I don't think it's useful enough that I'm going to > bother implementing it. If someone else wants to, I'd probably use it. > > > On Wed, Oct 14, 2009 at 4:47 PM, Darin Fisher <da...@chromium.org> wrote: > >> To be clear: I only weakly advocate Chrome having a SecurityOrigin. I'm >> also OKwith just using std::string. I think either is better than using >> GURL. >> >> -Darin >> >> >> On Wed, Oct 14, 2009 at 4:45 PM, Darin Fisher <da...@chromium.org> wrote: >> >>> Hmm... it seems useful as a means of making the code more >>> self-documenting anda bit safer. I'd rather not have people passing >>> strings for origins since they might be >>> tempted to parse the strings. >>> >>> The momentary translation to strings on the boundary of the WebKit API >>> does not >>> completely make this fragile. If the end-points in Chrome use >>> SecurityOrigin and >>> the end-points in the WebKit API use WebSecurityOrigin, then developers >>> will be >>> naturally inclined to convert between SecurityOrigin and >>> WebSecurityOrigin, >>> ignoring the toString() getter on WebSecurityOrigin. >>> >>> This is a case where existing code should help guide people in the right >>> direction. >>> >>> -Darin >>> >>> >>> >>> On Wed, Oct 14, 2009 at 4:39 PM, Jeremy Orlow <jor...@chromium.org>wrote: >>> >>>> 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 <da...@chromium.org>wrote: >>>> >>>>> >>>>> >>>>> 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 -~----------~----~----~----~------~----~------~--~---
