I guess you're right....about pretty much everything. I'll put it in chrome/common then? (Note I actually wasn't planning on putting it in base unless the API depended on it, but it seemed like including that would just make what I was saying more confusing.)
On Thu, Oct 15, 2009 at 12:19 AM, Darin Fisher <[email protected]> wrote: > 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 -~----------~----~----~----~------~----~------~--~---
