On Wed, Oct 14, 2009 at 7:17 PM, Jeremy Orlow <[email protected]> wrote:
> I think the main points of contention regarding SecurityOrigin::toString() > are the name "toString" and what type it should return. > > In terms of the names: I think we should provide constructors and getters > for each "component" of each WebKit API type. So, for example, > WebSecurityOrigin should have a constructor that takes in a single string > and a getter called |origin|. > It seems strange for something that is an origin to have a getter named origin. It leads to code like this: origin->origin(), which seems bad to me. toString() is the name used by WebCore, and it seems reasonable to me. It is merely a method that returns a string representation of the SecurityOrigin suitable for pickling. What is your objection to toString? > (We can argue about the name later.) base::SecurityOrigin > I mentioned before that base is not the right place for SecurityOrigin. See Brett's posts about what should and shouldn't be part of base/. It is a dumping ground today, and we should resist more of that will full force. In this case, only webkit/glue and above needs this type, so it does not need to live in base/. > can then have a method to produce WebSecurityOrigins (using that > constructor that takes in the base components....which is just "origin" in > this classes case) and it can have a constructor that takes in a > WebSecurityOrigin (which will construct itself by using the aforementioned > getter(s)). > This seems fine to me provided we actually need WebSecurityOrigin or SecurityOrigin to expose the component parts. However, I haven't seen any use cases for that yet. Maybe I have missed them? > > In terms of what it should return: Each "component" should really be a C++ > primitive. This of course leaves the question of how to handle strings. > Personally, I think we should do one of 2 things: Either WebString should > expose its components (an array of unsigned shorts + a size_t) > WebString already has accessors for the array of unsigned shorts and a size_t for the length. > or we should just put string16 into the WebKit API. The latter is not as > insane as it sounds: it's just a special case of std::string. The former > isn't that insane either since it can be fed directly to a std::string(16) > constructor. > The WebKit API is intended to be usable by consumers who do not have a full STL implementation (e.g., Android). > > If we add the proper constructors and factories for WebKit types to > NullableString16, GURL, SecurityOrigin, and any other types we might want to > connect to WebKit types in the future, then we can actually make the > dependency 100% one way. > It is OK for the WebKit API to know about GURL since WebCore depends on googleurl (specifically the generic, implementation bits). However, since GURL uses STL, there is just an optional (#ifdef controlled) conversion helper on WebURL for it. The same does not apply for NullableString16 or SecurityOrigin. > > I guess I feel like we should either say that depending on base types is OK > (as long as we think they'll be very stable implementation wise) or we > should say they're never allowed. This middle ground just feels wrong. > Note that the difference between toString and a single getter that returns > a string is subtle, but if all WebKit types follow this same convention, > then it's much less likely that people will do stupid things with them. > > I generally prefer all or nothing approaches too, but the implicit conversions to string16 and std::string are extremely nice. I don't want to give them up. At the same time, I object to building further dependencies on base from svn.webkit.org. NullableString16 seemed like a reasonable addition since it is just a slight variant of string16 that we can probably afford to treat as frozen. -Darin > J > > > On Wed, Oct 14, 2009 at 5:15 PM, Darin Fisher <[email protected]> wrote: > >> On Wed, Oct 14, 2009 at 4:51 PM, Jeremy Orlow <[email protected]>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 <[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 -~----------~----~----~----~------~----~------~--~---
