Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11026070:
Add API to construct new vector interchange MIME data type. (Closed)
|
Created:
8 years, 2 months ago by Greg Billock Modified:
8 years, 2 months ago CC:
chromium-reviews, groby+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, gbillock+watch_chromium.org, smckay+watch_chromium.org, rouslan+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd API to construct new vector interchange MIME data type.
Add test for web intents host.
R=groby@chromium.org
BUG=153695
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162239
Patch Set 1 #
Total comments: 12
Patch Set 2 : Factor out WebIntent creation #Patch Set 3 : Export symbol needed for test #
Total comments: 8
Patch Set 4 : Review comment changes #
Total comments: 8
Patch Set 5 : Use current v8 context #Patch Set 6 : Serialize list directly #Patch Set 7 : Switch to DidCreateScriptContext #
Total comments: 12
Patch Set 8 : Remove v8 scope creation #Patch Set 9 : Take out blob #
Total comments: 10
Patch Set 10 : Switch to switch #
Total comments: 4
Patch Set 11 : Improve comment #
Messages
Total messages: 44 (0 generated)
http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:195: // Do stuffs. You're fixing that comment, right? ;) http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:207: if (j == 0 && !intent_->blob_file.empty()) { This seems odd in here. Can we actually have blob data _and_ mime_data.GetSize() > 1? http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:221: web_intent = WebIntent::create(intent_->action, intent_->type, Can we take the last two lines out of all the separate branches? (I'd also feel more comfortable if the conversion routines were separate helpers that just returned a web_intent -> much better testability) http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... File content/renderer/web_intents_host.h (right): http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.h:65: friend class WebIntentsHostTest; Why not the more usual FRIEND_TEST_ALL_PREFIXES? http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... webkit/glue/web_intent_data.h:33: // Deprecated. Will be phased out in M25. Should we UMA usage of this? http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... webkit/glue/web_intent_data.h:98: Does WebIntentData need to be copy-constructed at all? If it does, please also implement operator= - copy ctor without assignment op usually leads to pain sooner or later. (Or at least make a private non-implementation)
http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:195: // Do stuffs. :-) On 2012/10/05 17:56:35, groby wrote: > You're fixing that comment, right? ;) http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:207: if (j == 0 && !intent_->blob_file.empty()) { Yes. It's just a struct. I'm not sure how else to do this for, say, RSS. We can't put the blob into the ListValue itself, it won't fit. Any better ideas? On 2012/10/05 17:56:35, groby wrote: > This seems odd in here. Can we actually have blob data _and_ mime_data.GetSize() > > 1? http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.cc:221: web_intent = WebIntent::create(intent_->action, intent_->type, Good point. Will do. On 2012/10/05 17:56:35, groby wrote: > Can we take the last two lines out of all the separate branches? > > (I'd also feel more comfortable if the conversion routines were separate helpers > that just returned a web_intent -> much better testability) http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... File content/renderer/web_intents_host.h (right): http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... content/renderer/web_intents_host.h:65: friend class WebIntentsHostTest; On 2012/10/05 17:56:35, groby wrote: > Why not the more usual FRIEND_TEST_ALL_PREFIXES? I had that, but I'm just calling it from the test fixture, so not needed. http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... webkit/glue/web_intent_data.h:33: // Deprecated. Will be phased out in M25. On 2012/10/05 17:56:35, groby wrote: > Should we UMA usage of this? Nah. We're going to kill it regardless. http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... webkit/glue/web_intent_data.h:98: Yes, good point. I'll private it out. On 2012/10/05 17:56:35, groby wrote: > Does WebIntentData need to be copy-constructed at all? > > If it does, please also implement operator= - copy ctor without assignment op > usually leads to pain sooner or later. (Or at least make a private > non-implementation)
On 2012/10/05 22:03:26, Greg Billock wrote: > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > File content/renderer/web_intents_host.cc (right): > > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > content/renderer/web_intents_host.cc:195: // Do stuffs. > :-) > > On 2012/10/05 17:56:35, groby wrote: > > You're fixing that comment, right? ;) > > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > content/renderer/web_intents_host.cc:207: if (j == 0 && > !intent_->blob_file.empty()) { > Yes. It's just a struct. I'm not sure how else to do this for, say, RSS. We > can't put the blob into the ListValue itself, it won't fit. Any better ideas? > > On 2012/10/05 17:56:35, groby wrote: > > This seems odd in here. Can we actually have blob data _and_ > mime_data.GetSize() > > > 1? > > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > content/renderer/web_intents_host.cc:221: web_intent = > WebIntent::create(intent_->action, intent_->type, > Good point. Will do. > > On 2012/10/05 17:56:35, groby wrote: > > Can we take the last two lines out of all the separate branches? > > > > (I'd also feel more comfortable if the conversion routines were separate > helpers > > that just returned a web_intent -> much better testability) > > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > File content/renderer/web_intents_host.h (right): > > http://codereview.chromium.org/11026070/diff/1/content/renderer/web_intents_h... > content/renderer/web_intents_host.h:65: friend class WebIntentsHostTest; > On 2012/10/05 17:56:35, groby wrote: > > Why not the more usual FRIEND_TEST_ALL_PREFIXES? > > I had that, but I'm just calling it from the test fixture, so not needed. > > http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h > File webkit/glue/web_intent_data.h (right): > > http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... > webkit/glue/web_intent_data.h:33: // Deprecated. Will be phased out in M25. > On 2012/10/05 17:56:35, groby wrote: > > Should we UMA usage of this? > > Nah. We're going to kill it regardless. > > http://codereview.chromium.org/11026070/diff/1/webkit/glue/web_intent_data.h#... > webkit/glue/web_intent_data.h:98: > Yes, good point. I'll private it out. > > On 2012/10/05 17:56:35, groby wrote: > > Does WebIntentData need to be copy-constructed at all? > > > > If it does, please also implement operator= - copy ctor without assignment op > > usually leads to pain sooner or later. (Or at least make a private > > non-implementation) +darin for webkit/glue; content/ OWNERS. Darin, does this method of adding the web_intents_host test look right?
http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:143: // Must be called with v8 scope held. Can we DCHECK for that? http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:200: return WebIntent::create(intent_data.action, intent_data.type, serialize/create is still shared across a lot of the code... I'd love it if we could factor that out. http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:236: return WebIntent(); Let's take out the whole else branch and just NOTREACHED/return? http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... File content/renderer/web_intents_host.h (right): http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.h:53: WebKit::WebFrame* frame, const webkit_glue::WebIntentData& intent_data); Don't you need an include file for WebIntent?
http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:143: // Must be called with v8 scope held. I think I got the right thing here now. On 2012/10/08 19:01:04, groby wrote: > Can we DCHECK for that? http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:200: return WebIntent::create(intent_data.action, intent_data.type, The trouble is the |data| -- I think we'll end up making extra copies if we factor it out. Still think we should? On 2012/10/08 19:01:04, groby wrote: > serialize/create is still shared across a lot of the code... I'd love it if we > could factor that out. http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.cc:236: return WebIntent(); On 2012/10/08 19:01:04, groby wrote: > Let's take out the whole else branch and just NOTREACHED/return? Done. http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... File content/renderer/web_intents_host.h (right): http://codereview.chromium.org/11026070/diff/5002/content/renderer/web_intent... content/renderer/web_intents_host.h:53: WebKit::WebFrame* frame, const webkit_glue::WebIntentData& intent_data); On 2012/10/08 19:01:04, groby wrote: > Don't you need an include file for WebIntent? Done.
LGTM (mod refactor) http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:82: WebIntentData(const string16& action_in, Both the proliferation of constructors, the partially unrelated data, and the large else branches elsewhere in this CL almost point to the fact that we want separate subclasses of WebIntentData. We should probably earmark this for a refactoring, at the very least.
http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data.h File webkit/glue/web_intent_data.h (right): http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data... webkit/glue/web_intent_data.h:82: WebIntentData(const string16& action_in, On 2012/10/09 20:55:14, groby wrote: > Both the proliferation of constructors, the partially unrelated data, and the > large else branches elsewhere in this CL almost point to the fact that we want > separate subclasses of WebIntentData. We should probably earmark this for a > refactoring, at the very least. This stems from not being able to do serialization in the browser process. That means we have to push all possible use case data to the renderer for prep there. Having subclasses is a good scheme to handle this, though. (Esp. if they can all serialize themselves...)
On 2012/10/09 21:25:17, Greg Billock wrote: > http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data.h > File webkit/glue/web_intent_data.h (right): > > http://codereview.chromium.org/11026070/diff/6013/webkit/glue/web_intent_data... > webkit/glue/web_intent_data.h:82: WebIntentData(const string16& action_in, > On 2012/10/09 20:55:14, groby wrote: > > Both the proliferation of constructors, the partially unrelated data, and the > > large else branches elsewhere in this CL almost point to the fact that we want > > separate subclasses of WebIntentData. We should probably earmark this for a > > refactoring, at the very least. > > This stems from not being able to do serialization in the browser process. That > means we have to push all possible use case data to the renderer for prep there. > Having subclasses is a good scheme to handle this, though. (Esp. if they can all > serialize themselves...) Ready for OWNERS look. Darin or James, can you look this over?
http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:137: WebIntent web_intent = CreateWebIntent(frame, *(intent_.get())); Why do you need to enter a V8 context before creating a web intent? Why is the main world the correct world to use? http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:214: v8::Local<v8::Context> ctx = frame->mainWorldScriptContext(); Do we need to call this API for every iteration of this loop? Why is the main world the correct world to use? http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:226: data_point->Set(blob_v8_key, web_blob_.toV8Value()); How do you know that this call to Set won't call a setter in the page and do arbitrary things in the middle of this loop?
http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:137: WebIntent web_intent = CreateWebIntent(frame, *(intent_.get())); On 2012/10/11 19:29:34, abarth wrote: > Why do you need to enter a V8 context before creating a web intent? Why is the > main world the correct world to use? The comment says "Returns the V8 context for this frame, or an empty handle if there is none." Since I'm delivering the intent to this frame, I think this is the right one to use. Since I'm just using this context to serialize data, will it matter though? http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:214: v8::Local<v8::Context> ctx = frame->mainWorldScriptContext(); On 2012/10/11 19:29:34, abarth wrote: > Do we need to call this API for every iteration of this loop? Why is the main > world the correct world to use? This needs to be the same as the current context. Changed to make that more correct. http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... content/renderer/web_intents_host.cc:226: data_point->Set(blob_v8_key, web_blob_.toV8Value()); On 2012/10/11 19:29:34, abarth wrote: > How do you know that this call to Set won't call a setter in the page and do > arbitrary things in the middle of this loop? This is data-for-serialization that I'm creating. There's no way for other code to have knowledge of it until I deliver it below. The basic goal is to get this browser-created data into serialized form for consumption by the WebIntent payload. We can't serialize in the browser process, so it gets passed here natively, where we have the right v8 contexts available to do the job. I am going to switch to just use the v8 value converter list functionality.
On 2012/10/11 19:48:53, Greg Billock wrote: > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > File content/renderer/web_intents_host.cc (right): > > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > content/renderer/web_intents_host.cc:137: WebIntent web_intent = > CreateWebIntent(frame, *(intent_.get())); > On 2012/10/11 19:29:34, abarth wrote: > > Why do you need to enter a V8 context before creating a web intent? Why is > the > > main world the correct world to use? > > The comment says "Returns the V8 context for this frame, or an empty handle if > there is none." Since I'm delivering the intent to this frame, I think this is > the right one to use. Unfortunately, life is a bit more complicated than that. There are potentially many v8::Contexts associated with a given frame, depending on whether there are content scripts running in that frame. > Since I'm just using this context to serialize data, will it matter though? If it doesn't matter, please remove it and teach WebKit how to serialize the data without needing the caller to supply a v8::Context. > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > content/renderer/web_intents_host.cc:226: data_point->Set(blob_v8_key, > web_blob_.toV8Value()); > On 2012/10/11 19:29:34, abarth wrote: > > How do you know that this call to Set won't call a setter in the page and do > > arbitrary things in the middle of this loop? > > This is data-for-serialization that I'm creating. There's no way for other code > to have knowledge of it until I deliver it below. What stops the web page from adding a setter to Object.prototype? Will that trigger in this code? > The basic goal is to get this browser-created data into serialized form for > consumption by the WebIntent payload. We can't serialize in the browser process, > so it gets passed here natively, where we have the right v8 contexts available > to do the job. The code as written looks wrong. Its likely there's a better way of achieving this goal that is correct.
I ran a quick test:
Object.prototype.__defineSetter__("foo", function() { alert(99) });
var ttt = {};
ttt.foo = "bar";
This code ends up calling alert(99).
On 2012/10/11 20:03:52, abarth wrote: > On 2012/10/11 19:48:53, Greg Billock wrote: > > > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > > File content/renderer/web_intents_host.cc (right): > > > > > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > > content/renderer/web_intents_host.cc:137: WebIntent web_intent = > > CreateWebIntent(frame, *(intent_.get())); > > On 2012/10/11 19:29:34, abarth wrote: > > > Why do you need to enter a V8 context before creating a web intent? Why is > > the > > > main world the correct world to use? > > > > The comment says "Returns the V8 context for this frame, or an empty handle if > > there is none." Since I'm delivering the intent to this frame, I think this is > > the right one to use. > > Unfortunately, life is a bit more complicated than that. There are potentially > many v8::Contexts associated with a given frame, depending on whether there are > content scripts running in that frame. > > > Since I'm just using this context to serialize data, will it matter though? > > If it doesn't matter, please remove it and teach WebKit how to serialize the > data without needing the caller to supply a v8::Context. There's an existing method to do serialization -- it's really the conversion and manipulation of the v8 object I need do to get ready for that call which is the problem. Representing what I need to do there within webkit is certainly possible -- the various cases I deal with here could be handled within WebIntent itself, for instance. My goal has been to keep that interface a lot smaller, though, and only speak WebSerializedScriptValue. > > > http://codereview.chromium.org/11026070/diff/6013/content/renderer/web_intent... > > content/renderer/web_intents_host.cc:226: data_point->Set(blob_v8_key, > > web_blob_.toV8Value()); > > On 2012/10/11 19:29:34, abarth wrote: > > > How do you know that this call to Set won't call a setter in the page and do > > > arbitrary things in the middle of this loop? > > > > This is data-for-serialization that I'm creating. There's no way for other > code > > to have knowledge of it until I deliver it below. > > What stops the web page from adding a setter to Object.prototype? Will that > trigger in this code? This code gets run upon first creation of the 'window' object -- I believe that precedes any possible such modification by the page. Regardless of this, though, I certainly don't expect or want it to, which I think is your point, right? > > The basic goal is to get this browser-created data into serialized form for > > consumption by the WebIntent payload. We can't serialize in the browser > process, > > so it gets passed here natively, where we have the right v8 contexts available > > to do the job. > > The code as written looks wrong. Its likely there's a better way of achieving > this goal that is correct. There's the WebSerializedScriptValue method to serialize the v8 object, which I'm using. Ultimately, though, I think the problem is that I need a local v8 scope to make said objects. Is there an isolate that's better to use for the context for this kind of thing? Looking about in the code, both when I wrote this and now, it looks to me like we use this mainWorldScriptContext for such operations. Can I instead just use v8::Context::New like in some tests? That'd be way nicer.
It's very unlikely that you should be creating a new v8::Context for this work. All the code that I've found in Chromium that creates a new V8::Context is wrong and can lead to security vulnerabilities. By a similar token, using mainWorldScriptContext is also very unlikely to be correct since it won't work properly with isolated worlds. I'm sorry that I don't have time right now to sort out the right design in the middle of this code review. It's like we'd need to talk this through at a design level first.
If I had to guess, though, I would guess that the correct design here involves removing all uses of the V8 API from this code.
Agreed it would be nicer not to touch the v8 api here. Since we aren't delivering intents to content scripts, I think that mainWorldScriptContext is the right one to use. Shall we go ahead with this for now and figure out how to remove the v8 api later? Or is there already a better way to do this? On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: > It's very unlikely that you should be creating a new v8::Context for this > work. > All the code that I've found in Chromium that creates a new V8::Context is > wrong > and can lead to security vulnerabilities. By a similar token, using > mainWorldScriptContext is also very unlikely to be correct since it won't > work > properly with isolated worlds. > > I'm sorry that I don't have time right now to sort out the right design in > the > middle of this code review. It's like we'd need to talk this through at a > design level first. > > http://codereview.chromium.org/11026070/
Our experience with "go with this for now" and clean up later has been pretty poor, but I don't have enough context to answer your question. On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <gbillock@chromium.org> wrote: > Agreed it would be nicer not to touch the v8 api here. Since we aren't > delivering intents to content scripts, I think that > mainWorldScriptContext is the right one to use. > > Shall we go ahead with this for now and figure out how to remove the > v8 api later? Or is there already a better way to do this? > > On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: >> It's very unlikely that you should be creating a new v8::Context for this >> work. >> All the code that I've found in Chromium that creates a new V8::Context is >> wrong >> and can lead to security vulnerabilities. By a similar token, using >> mainWorldScriptContext is also very unlikely to be correct since it won't >> work >> properly with isolated worlds. >> >> I'm sorry that I don't have time right now to sort out the right design in >> the >> middle of this code review. It's like we'd need to talk this through at a >> design level first. >> >> http://codereview.chromium.org/11026070/
:-) I hear you. Let's talk tomorrow. Here's an idea: Looking at the context, the DidClearWindowObject method is getting forwarded from dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*), and the argument is getting ignored. Should this method really be accepting a v8::Context to the JS world? That doesn't address everything you've said, but it places the origin for the v8 context in the caller, which is a lot nicer and fits the API better. Even further along those lines, this call could be specified to be made with a scope set -- it's a natural place to bind objects, and that's mostly what we do, and that's where being in a v8 scope would be convenient. It's quite opaque, but gets the job done. An alternate plan is to forward didCreateScriptContext to observers, which already passes this context. That'd only require chromium-side changes. These approach would leave some interaction with the v8 api in place, but I think that's going to be harder to cleanly solve -- marshalling these objects lower down will still require the v8 context. If there's another spot this is done better, let me know. On Thu, Oct 11, 2012 at 2:43 PM, Adam Barth <abarth@chromium.org> wrote: > Our experience with "go with this for now" and clean up later has been > pretty poor, but I don't have enough context to answer your question. > > > On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <gbillock@chromium.org> wrote: >> Agreed it would be nicer not to touch the v8 api here. Since we aren't >> delivering intents to content scripts, I think that >> mainWorldScriptContext is the right one to use. >> >> Shall we go ahead with this for now and figure out how to remove the >> v8 api later? Or is there already a better way to do this? >> >> On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: >>> It's very unlikely that you should be creating a new v8::Context for this >>> work. >>> All the code that I've found in Chromium that creates a new V8::Context is >>> wrong >>> and can lead to security vulnerabilities. By a similar token, using >>> mainWorldScriptContext is also very unlikely to be correct since it won't >>> work >>> properly with isolated worlds. >>> >>> I'm sorry that I don't have time right now to sort out the right design in >>> the >>> middle of this code review. It's like we'd need to talk this through at a >>> design level first. >>> >>> http://codereview.chromium.org/11026070/
DidClearWindowObject doesn't work correctly with isolated worlds yet. We're working on fixing it, but there are some difficult issues to resolve. didCreateScriptContext might be a more sensible time to do some of this work. Adam On Thu, Oct 11, 2012 at 9:39 PM, Greg Billock <gbillock@chromium.org> wrote: > :-) I hear you. Let's talk tomorrow. Here's an idea: > > Looking at the context, the DidClearWindowObject method is getting > forwarded from dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*), > and the argument is getting ignored. Should this method really be > accepting a v8::Context to the JS world? That doesn't address > everything you've said, but it places the origin for the v8 context in > the caller, which is a lot nicer and fits the API better. Even further > along those lines, this call could be specified to be made with a > scope set -- it's a natural place to bind objects, and that's mostly > what we do, and that's where being in a v8 scope would be convenient. > It's quite opaque, but gets the job done. > > An alternate plan is to forward didCreateScriptContext to observers, > which already passes this context. That'd only require chromium-side > changes. > > These approach would leave some interaction with the v8 api in place, > but I think that's going to be harder to cleanly solve -- marshalling > these objects lower down will still require the v8 context. If there's > another spot this is done better, let me know. > > > > On Thu, Oct 11, 2012 at 2:43 PM, Adam Barth <abarth@chromium.org> wrote: >> Our experience with "go with this for now" and clean up later has been >> pretty poor, but I don't have enough context to answer your question. >> >> >> On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <gbillock@chromium.org> wrote: >>> Agreed it would be nicer not to touch the v8 api here. Since we aren't >>> delivering intents to content scripts, I think that >>> mainWorldScriptContext is the right one to use. >>> >>> Shall we go ahead with this for now and figure out how to remove the >>> v8 api later? Or is there already a better way to do this? >>> >>> On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: >>>> It's very unlikely that you should be creating a new v8::Context for this >>>> work. >>>> All the code that I've found in Chromium that creates a new V8::Context is >>>> wrong >>>> and can lead to security vulnerabilities. By a similar token, using >>>> mainWorldScriptContext is also very unlikely to be correct since it won't >>>> work >>>> properly with isolated worlds. >>>> >>>> I'm sorry that I don't have time right now to sort out the right design in >>>> the >>>> middle of this code review. It's like we'd need to talk this through at a >>>> design level first. >>>> >>>> http://codereview.chromium.org/11026070/
Sounds good. Shall I send you a patch to propagate didCreateScriptContext? Or shall we just special-forward it to intents_host_ for now? On Thu, Oct 11, 2012 at 9:41 PM, Adam Barth <abarth@chromium.org> wrote: > DidClearWindowObject doesn't work correctly with isolated worlds yet. > We're working on fixing it, but there are some difficult issues to > resolve. > > didCreateScriptContext might be a more sensible time to do some of this work. > > Adam > > On Thu, Oct 11, 2012 at 9:39 PM, Greg Billock <gbillock@chromium.org> wrote: >> :-) I hear you. Let's talk tomorrow. Here's an idea: >> >> Looking at the context, the DidClearWindowObject method is getting >> forwarded from dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*), >> and the argument is getting ignored. Should this method really be >> accepting a v8::Context to the JS world? That doesn't address >> everything you've said, but it places the origin for the v8 context in >> the caller, which is a lot nicer and fits the API better. Even further >> along those lines, this call could be specified to be made with a >> scope set -- it's a natural place to bind objects, and that's mostly >> what we do, and that's where being in a v8 scope would be convenient. >> It's quite opaque, but gets the job done. >> >> An alternate plan is to forward didCreateScriptContext to observers, >> which already passes this context. That'd only require chromium-side >> changes. >> >> These approach would leave some interaction with the v8 api in place, >> but I think that's going to be harder to cleanly solve -- marshalling >> these objects lower down will still require the v8 context. If there's >> another spot this is done better, let me know. >> >> >> >> On Thu, Oct 11, 2012 at 2:43 PM, Adam Barth <abarth@chromium.org> wrote: >>> Our experience with "go with this for now" and clean up later has been >>> pretty poor, but I don't have enough context to answer your question. >>> >>> >>> On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <gbillock@chromium.org> wrote: >>>> Agreed it would be nicer not to touch the v8 api here. Since we aren't >>>> delivering intents to content scripts, I think that >>>> mainWorldScriptContext is the right one to use. >>>> >>>> Shall we go ahead with this for now and figure out how to remove the >>>> v8 api later? Or is there already a better way to do this? >>>> >>>> On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: >>>>> It's very unlikely that you should be creating a new v8::Context for this >>>>> work. >>>>> All the code that I've found in Chromium that creates a new V8::Context is >>>>> wrong >>>>> and can lead to security vulnerabilities. By a similar token, using >>>>> mainWorldScriptContext is also very unlikely to be correct since it won't >>>>> work >>>>> properly with isolated worlds. >>>>> >>>>> I'm sorry that I don't have time right now to sort out the right design in >>>>> the >>>>> middle of this code review. It's like we'd need to talk this through at a >>>>> design level first. >>>>> >>>>> http://codereview.chromium.org/11026070/
You'll probably need to send the didCreateScriptContext patch to someone else. I'm not an OWNER for anything in Chromium these days. Adam On Thu, Oct 11, 2012 at 9:49 PM, Greg Billock <gbillock@chromium.org> wrote: > Sounds good. Shall I send you a patch to propagate > didCreateScriptContext? Or shall we just special-forward it to > intents_host_ for now? > > On Thu, Oct 11, 2012 at 9:41 PM, Adam Barth <abarth@chromium.org> wrote: >> DidClearWindowObject doesn't work correctly with isolated worlds yet. >> We're working on fixing it, but there are some difficult issues to >> resolve. >> >> didCreateScriptContext might be a more sensible time to do some of this work. >> >> Adam >> >> On Thu, Oct 11, 2012 at 9:39 PM, Greg Billock <gbillock@chromium.org> wrote: >>> :-) I hear you. Let's talk tomorrow. Here's an idea: >>> >>> Looking at the context, the DidClearWindowObject method is getting >>> forwarded from dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*), >>> and the argument is getting ignored. Should this method really be >>> accepting a v8::Context to the JS world? That doesn't address >>> everything you've said, but it places the origin for the v8 context in >>> the caller, which is a lot nicer and fits the API better. Even further >>> along those lines, this call could be specified to be made with a >>> scope set -- it's a natural place to bind objects, and that's mostly >>> what we do, and that's where being in a v8 scope would be convenient. >>> It's quite opaque, but gets the job done. >>> >>> An alternate plan is to forward didCreateScriptContext to observers, >>> which already passes this context. That'd only require chromium-side >>> changes. >>> >>> These approach would leave some interaction with the v8 api in place, >>> but I think that's going to be harder to cleanly solve -- marshalling >>> these objects lower down will still require the v8 context. If there's >>> another spot this is done better, let me know. >>> >>> >>> >>> On Thu, Oct 11, 2012 at 2:43 PM, Adam Barth <abarth@chromium.org> wrote: >>>> Our experience with "go with this for now" and clean up later has been >>>> pretty poor, but I don't have enough context to answer your question. >>>> >>>> >>>> On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <gbillock@chromium.org> wrote: >>>>> Agreed it would be nicer not to touch the v8 api here. Since we aren't >>>>> delivering intents to content scripts, I think that >>>>> mainWorldScriptContext is the right one to use. >>>>> >>>>> Shall we go ahead with this for now and figure out how to remove the >>>>> v8 api later? Or is there already a better way to do this? >>>>> >>>>> On Thu, Oct 11, 2012 at 2:12 PM, <abarth@chromium.org> wrote: >>>>>> It's very unlikely that you should be creating a new v8::Context for this >>>>>> work. >>>>>> All the code that I've found in Chromium that creates a new V8::Context is >>>>>> wrong >>>>>> and can lead to security vulnerabilities. By a similar token, using >>>>>> mainWorldScriptContext is also very unlikely to be correct since it won't >>>>>> work >>>>>> properly with isolated worlds. >>>>>> >>>>>> I'm sorry that I don't have time right now to sort out the right design in >>>>>> the >>>>>> middle of this code review. It's like we'd need to talk this through at a >>>>>> design level first. >>>>>> >>>>>> http://codereview.chromium.org/11026070/
On 2012/10/12 05:24:18, abarth wrote: > You'll probably need to send the didCreateScriptContext patch to > someone else. I'm not an OWNER for anything in Chromium these days. Just folded it in here. Does this look better? Getting rid of making the context is a definite improvement. > On Thu, Oct 11, 2012 at 9:49 PM, Greg Billock <mailto:gbillock@chromium.org> wrote: > > Sounds good. Shall I send you a patch to propagate > > didCreateScriptContext? Or shall we just special-forward it to > > intents_host_ for now? > > > > On Thu, Oct 11, 2012 at 9:41 PM, Adam Barth <mailto:abarth@chromium.org> wrote: > >> DidClearWindowObject doesn't work correctly with isolated worlds yet. > >> We're working on fixing it, but there are some difficult issues to > >> resolve. > >> > >> didCreateScriptContext might be a more sensible time to do some of this > work. > >> > >> Adam > >> > >> On Thu, Oct 11, 2012 at 9:39 PM, Greg Billock <mailto:gbillock@chromium.org> wrote: > >>> :-) I hear you. Let's talk tomorrow. Here's an idea: > >>> > >>> Looking at the context, the DidClearWindowObject method is getting > >>> forwarded from dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*), > >>> and the argument is getting ignored. Should this method really be > >>> accepting a v8::Context to the JS world? That doesn't address > >>> everything you've said, but it places the origin for the v8 context in > >>> the caller, which is a lot nicer and fits the API better. Even further > >>> along those lines, this call could be specified to be made with a > >>> scope set -- it's a natural place to bind objects, and that's mostly > >>> what we do, and that's where being in a v8 scope would be convenient. > >>> It's quite opaque, but gets the job done. > >>> > >>> An alternate plan is to forward didCreateScriptContext to observers, > >>> which already passes this context. That'd only require chromium-side > >>> changes. > >>> > >>> These approach would leave some interaction with the v8 api in place, > >>> but I think that's going to be harder to cleanly solve -- marshalling > >>> these objects lower down will still require the v8 context. If there's > >>> another spot this is done better, let me know. > >>> > >>> > >>> > >>> On Thu, Oct 11, 2012 at 2:43 PM, Adam Barth <mailto:abarth@chromium.org> wrote: > >>>> Our experience with "go with this for now" and clean up later has been > >>>> pretty poor, but I don't have enough context to answer your question. > >>>> > >>>> > >>>> On Thu, Oct 11, 2012 at 2:34 PM, Greg Billock <mailto:gbillock@chromium.org> > wrote: > >>>>> Agreed it would be nicer not to touch the v8 api here. Since we aren't > >>>>> delivering intents to content scripts, I think that > >>>>> mainWorldScriptContext is the right one to use. > >>>>> > >>>>> Shall we go ahead with this for now and figure out how to remove the > >>>>> v8 api later? Or is there already a better way to do this? > >>>>> > >>>>> On Thu, Oct 11, 2012 at 2:12 PM, <mailto:abarth@chromium.org> wrote: > >>>>>> It's very unlikely that you should be creating a new v8::Context for > this > >>>>>> work. > >>>>>> All the code that I've found in Chromium that creates a new V8::Context > is > >>>>>> wrong > >>>>>> and can lead to security vulnerabilities. By a similar token, using > >>>>>> mainWorldScriptContext is also very unlikely to be correct since it > won't > >>>>>> work > >>>>>> properly with isolated worlds. > >>>>>> > >>>>>> I'm sorry that I don't have time right now to sort out the right design > in > >>>>>> the > >>>>>> middle of this code review. It's like we'd need to talk this through at > a > >>>>>> design level first. > >>>>>> > >>>>>> http://codereview.chromium.org/11026070/
This definitely seems like an improvement since we're not longer assuming that the main world is the only one that matters. Do we want to do this processing for every world? Is the processing safe to do multiple times per frame? http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:135: v8::Context::Scope cscope(ctx); Do we need either of these things any more? We should already have a HandleScope on the stack and we should also already be in the proper context. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:145: DCHECK(!v8::Context::GetCurrent().IsEmpty()); Why not call v8::Context::InContext() ? That's the canonical way to check whether we're in a context. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:166: reinterpret_cast<const uint16_t*>(intent_data.unserialized_data.data()), Is this safe? Do we need to worry about unpaired surrogates or other Unicode gotchas? Generally, we prefer to use array buffers rather than strings for binary data. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:176: WebString::fromUTF8(intent_data.blob_file.AsUTF8Unsafe()), What if the blob_file cannot be represented as UTF8? http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:217: WebString::fromUTF8(intent_data.blob_file.AsUTF8Unsafe()), Should we be concerned that we're calling a function with "unsafe" in the name? http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:220: web_blob_.toV8Value()); It's not clear to me why it is safe to call Set here. Also, why are we creating a string each time instead of using a single v8::String that we hold in a local handle.
Does this happen for every frame, or is the work limited to a single frame (e.g., the top-level frame)?
http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:135: v8::Context::Scope cscope(ctx); On 2012/10/12 19:59:09, abarth wrote: > Do we need either of these things any more? We should already have a > HandleScope on the stack and we should also already be in the proper context. Yes. Just checked and this gets called within these scopes. Good deal! http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:145: DCHECK(!v8::Context::GetCurrent().IsEmpty()); On 2012/10/12 19:59:09, abarth wrote: > Why not call v8::Context::InContext() ? That's the canonical way to check > whether we're in a context. Done. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:166: reinterpret_cast<const uint16_t*>(intent_data.unserialized_data.data()), On 2012/10/12 19:59:09, abarth wrote: > Is this safe? Do we need to worry about unpaired surrogates or other Unicode > gotchas? > > Generally, we prefer to use array buffers rather than strings for binary data. This is for text data prepared safely in the browser process. For example, with url.spec(). Binary data is prepared in other ways (there's support for blob, but not array buffers currently). http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:176: WebString::fromUTF8(intent_data.blob_file.AsUTF8Unsafe()), On 2012/10/12 19:59:09, abarth wrote: > What if the blob_file cannot be represented as UTF8? This refers to the name not having encoding information included in some POSIX systems. If I read the code correctly, this'll end up in an empty blob. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:217: WebString::fromUTF8(intent_data.blob_file.AsUTF8Unsafe()), On 2012/10/12 19:59:09, abarth wrote: > Should we be concerned that we're calling a function with "unsafe" in the name? See above. I think this is the best way to translate a filename from FilePath down to webkit. http://codereview.chromium.org/11026070/diff/34001/content/renderer/web_inten... content/renderer/web_intents_host.cc:220: web_blob_.toV8Value()); On 2012/10/12 19:59:09, abarth wrote: > It's not clear to me why it is safe to call Set here. Because of the __setter possibility? I looked through the v8 code, and I do think it has at least the possibility of going through that path. But see V8ValueConverterImpl, for instance. It uses v8::Object::New, and then result->Set on the resulting handle. Do those get run asynchronously, so it's not a problem, or are (some? all?) uses of this serializer suspect? > Also, why are we creating > a string each time instead of using a single v8::String that we hold in a local > handle. You think that optimization is needed? I thought this was easier to read.
> But see V8ValueConverterImpl, for instance. It uses v8::Object::New, and then > result->Set on the resulting handle. Do those get run asynchronously, so it's > not a problem, or are (some? all?) uses of this serializer suspect? I suspect V8ValueConverter is also completely wrong. Code that uses V8 in Chromium doesn't get a high quality review because the folks who understand the API mostly work on WebKit. > > Also, why are we creating > > a string each time instead of using a single v8::String that we hold in a > local > > handle. > > You think that optimization is needed? I thought this was easier to read. It's just sloppy coding.
On 2012/10/12 19:59:59, abarth wrote: > Does this happen for every frame, or is the work limited to a single frame > (e.g., the top-level frame)? Didn't reply earlier because I was still thinking about whether we want to deliver to all worlds in the top frame. We want to limit to top-level, so there's a check for that. I added a check for the main world as well. That'll not deliver to any content scripts (are we using isolates for anything else now?). I think that's ok, but we may revisit this at some point.
We might also use them for the web insector, but not delivering sounds like the right thing. On Sat, Oct 13, 2012 at 12:47 AM, <gbillock@chromium.org> wrote: > On 2012/10/12 19:59:59, abarth wrote: >> >> Does this happen for every frame, or is the work limited to a single frame >> (e.g., the top-level frame)? > > > Didn't reply earlier because I was still thinking about whether we want to > deliver to all worlds in the top frame. We want to limit to top-level, so > there's a check for that. I added a check for the main world as well. > That'll > not deliver to any content scripts (are we using isolates for anything else > now?). I think that's ok, but we may revisit this at some point. > > http://codereview.chromium.org/11026070/
On 2012/10/12 21:37:21, abarth wrote: > > But see V8ValueConverterImpl, for instance. It uses v8::Object::New, and then > > result->Set on the resulting handle. Do those get run asynchronously, so it's > > not a problem, or are (some? all?) uses of this serializer suspect? > > I suspect V8ValueConverter is also completely wrong. Code that uses V8 in > Chromium doesn't get a high quality review because the folks who understand the > API mostly work on WebKit. That could be. Shall I add a TODO to that class to get it re-reviewed? I appreciate that the v8 api is tricky to use right. Currently that api is the best way I know of to manipulate JS values from chromium code. > > > > Also, why are we creating > > > a string each time instead of using a single v8::String that we hold in a > > local > > > handle. > > > > You think that optimization is needed? I thought this was easier to read. > > It's just sloppy coding. I ended up just taking this Blob handling out for now. I think this was clearer after changing how I used the converter, but this value only gets used once per context, and likely only once per renderer/WebIntentsHost, so I think it makes for poorer readability to pull it out with no offsetting gain I can see. It isn't clear from the API that using a persistent handle or something like that is going to end up being beneficial for v8 internals anyway.
Thanks for iterating on this patch. The use of the V8 API in this patch LGTM.
On 2012/10/13 08:01:24, abarth wrote: > Thanks for iterating on this patch. The use of the V8 API in this patch LGTM. Thanks! Darin, can you take a look for owners?
http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:137: WebIntent web_intent = CreateWebIntent(frame, *(intent_.get())); scoped_ptr<> has an operator* so you can say just "*intent_" instead of "*(intent_.get())" http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:153: for (i = 0, iter = intent_data.extra_data.begin(); nit: i would scope the loop counter and iterator inside the for loop since they don't appear to be used for the rest of the (long) function http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:160: if (intent_data.data_type == webkit_glue::WebIntentData::SERIALIZED) { This big else-if ladder chain should be a switch statement. Then the compiler could tell you if you were missing a case instead of only having the runtime NOT_REACHED() at the bottom of the function http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:162: intent_data.data, this doesn't appear to be aligned correctly http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:167: reinterpret_cast<const uint16_t*>(intent_data.unserialized_data.data()), I'm a bit surprised by this reinterpret_cast<> - unserialized_data is a string16 aka basic_string<char16...> and char16 is uint16_t everywhere, so why does this need the reinterpret?
http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... File content/renderer/web_intents_host.cc (right): http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:137: WebIntent web_intent = CreateWebIntent(frame, *(intent_.get())); On 2012/10/15 01:58:29, jamesr wrote: > scoped_ptr<> has an operator* so you can say just "*intent_" instead of > "*(intent_.get())" Done. http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:153: for (i = 0, iter = intent_data.extra_data.begin(); On 2012/10/15 01:58:29, jamesr wrote: > nit: i would scope the loop counter and iterator inside the for loop since they > don't appear to be used for the rest of the (long) function The compiler gets confused that the two initializations aren't the same type. Moved the size_t one inline. http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:160: if (intent_data.data_type == webkit_glue::WebIntentData::SERIALIZED) { On 2012/10/15 01:58:29, jamesr wrote: > This big else-if ladder chain should be a switch statement. Then the compiler > could tell you if you were missing a case instead of only having the runtime > NOT_REACHED() at the bottom of the function Done. http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:162: intent_data.data, On 2012/10/15 01:58:29, jamesr wrote: > this doesn't appear to be aligned correctly Weird. It shows up right in my editor. Deletion doesn't show any niggly tabs. Deleted and replaced all spaces anyway, we'll see if that helps... http://codereview.chromium.org/11026070/diff/44001/content/renderer/web_inten... content/renderer/web_intents_host.cc:167: reinterpret_cast<const uint16_t*>(intent_data.unserialized_data.data()), On 2012/10/15 01:58:29, jamesr wrote: > I'm a bit surprised by this reinterpret_cast<> - unserialized_data is a string16 > aka basic_string<char16...> and char16 is uint16_t everywhere, so why does this > need the reinterpret? Agreed this is unexpected. The windows compiler gets confused by this, and can't decide which v8::String::New to use.
webkit/glue and content/renderer lgtm
Drive by nits. https://codereview.chromium.org/11026070/diff/53001/content/browser/intents/i... File content/browser/intents/intent_injector.cc (right): https://codereview.chromium.org/11026070/diff/53001/content/browser/intents/i... content/browser/intents/intent_injector.cc:85: if (source_intent_->data_type == webkit_glue::WebIntentData::BLOB || The checks are crystal clear, why you are doing them is not. Can you add a code comment? Also, is there a test that should be updated to verify the new rules? https://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inte... File content/renderer/web_intents_host.h (right): https://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inte... content/renderer/web_intents_host.h:50: virtual void DidCreateScriptContext(WebKit::WebFrame* frame, Odd phrasing. Maybe: OnScriptContextCreated?
https://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inte... > File content/renderer/web_intents_host.h (right): > > https://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inte... > content/renderer/web_intents_host.h:50: virtual void > DidCreateScriptContext(WebKit::WebFrame* frame, > Odd phrasing. Maybe: > > OnScriptContextCreated? OnScriptContextCreated is unclear about whether the context has been created yet or not. Did...() is unambiguous - it's after. Will..() is used for hooks that happen before ...
http://codereview.chromium.org/11026070/diff/53001/content/browser/intents/in... File content/browser/intents/intent_injector.cc (right): http://codereview.chromium.org/11026070/diff/53001/content/browser/intents/in... content/browser/intents/intent_injector.cc:85: if (source_intent_->data_type == webkit_glue::WebIntentData::BLOB || On 2012/10/15 19:31:10, Steve McKay wrote: > The checks are crystal clear, why you are doing them is not. Can you add a code > comment? > > Also, is there a test that should be updated to verify the new rules? The new WebIntentsHost will grow to accomodate this on the encoding side. I plan to make a content unit test for this file, as well. http://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inten... File content/renderer/web_intents_host.h (right): http://codereview.chromium.org/11026070/diff/53001/content/renderer/web_inten... content/renderer/web_intents_host.h:50: virtual void DidCreateScriptContext(WebKit::WebFrame* frame, On 2012/10/15 19:31:10, Steve McKay wrote: > Odd phrasing. Maybe: > > OnScriptContextCreated? This echoes the existing API method in RenderViewImpl and the WebKit/chromium API.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11026070/43002
Presubmit check for 11026070-43002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content
Presubmit checks took 4.3s to calculate.
On 2012/10/15 21:02:36, I haz the power (commit-bot) wrote: > Presubmit check for 11026070-43002 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > content > > Presubmit checks took 4.3s to calculate. +jam for content/ gypi approval
On 2012/10/15 21:41:06, Greg Billock wrote: > On 2012/10/15 21:02:36, I haz the power (commit-bot) wrote: > > Presubmit check for 11026070-43002 failed and returned exit status 1. > > > > > > Running presubmit commit checks ... > > > > ** Presubmit ERRORS ** > > Missing LGTM from an OWNER for files in these directories: > > content > > > > Presubmit checks took 4.3s to calculate. > > +jam for content/ gypi approval lgtm.. btw for future expediency, feel free to tbr trivial changes to gypi files or send to other owners
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11026070/43002
Change committed as 162239 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
