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|. (We can argue about the name later.) base::SecurityOrigin 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)). 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) 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. 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. 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. J On Wed, Oct 14, 2009 at 5:15 PM, Darin Fisher <da...@chromium.org> wrote: > 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 -~----------~----~----~----~------~----~------~--~---
