You mean string16, right? 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.
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>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 <[email protected]>wrote: >>> >>>> >>>> >>>> On Wed, Oct 14, 2009 at 3:59 PM, Jeremy Orlow <[email protected]>wrote: >>>> >>>>> On Wed, Oct 14, 2009 at 2:48 PM, Darin Fisher <[email protected]>wrote: >>>>> >>>>>> On Wed, Oct 14, 2009 at 2:47 PM, Darin Fisher <[email protected]>wrote: >>>>>> >>>>>>> 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. >>>>>>> >>>>>> >>>>> 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: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
