Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
878513005:
Extensions: suspend extension's scripts when V8 is paused (Closed)
|
Created:
5 years, 10 months ago by kozy Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: suspend extension's scripts when V8 is paused
This CL replaced sync script execution API with async.
Async API suspends extension's scripts when V8 is paused at breakpoint or when nested event loop is running for modal dialogs otherwise scripts are executed synchronously.
API provides guarantees of execution order (first in - first execute, per frame).
BUG=410289
R=vsevik@chromium.org
Committed: https://crrev.com/c8bc9a58dc08a98fe889c269b680ead30558d372
Cr-Commit-Position: refs/heads/master@{#319425}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : rebased #
Total comments: 2
Patch Set 5 : check added #
Total comments: 4
Patch Set 6 : #Patch Set 7 : Fixed ActivityLogPrerenderTest.TestScriptInjected #
Total comments: 10
Patch Set 8 : Addressed comments #
Total comments: 56
Patch Set 9 : #
Total comments: 26
Patch Set 10 : Addressed comments #
Total comments: 12
Patch Set 11 #
Total comments: 2
Patch Set 12 : #
Total comments: 10
Patch Set 13 : #
Total comments: 2
Messages
Total messages: 49 (10 generated)
Vsevolod, please take a look!
CL depends on https://codereview.chromium.org/869513004/. Vsevolod, please take a look!
Vsevolod, friendly ping!
Looks good, please make sure someone more familiar with extensions confirms that 1) scripts_run_info.LogRun call inside ScriptInjectionManager::HandleExecuteCode is correct 2) the extension invalidating logic is not really neccessary (invalidated_while_injecting_ can be safely removed)
https://codereview.chromium.org/878513005/diff/60001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/60001/extensions/renderer/scri... extensions/renderer/script_injection.cc:335: if (script_injection_manager_) { This seems impossible, assert instead?
https://codereview.chromium.org/878513005/diff/60001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/60001/extensions/renderer/scri... extensions/renderer/script_injection.cc:335: if (script_injection_manager_) { On 2015/02/06 14:33:52, vsevik wrote: > This seems impossible, assert instead? Done.
kozyatinskiy@chromium.org changed reviewers: + kalman@chromium.org
@kalman, please take a look.
kalman@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin is a better reviewer than me.
On 2015/02/10 22:53:44, kalman wrote: > Devlin is a better reviewer than me. Can you expand a bit on the CL description and the bug? In particular, does this only suspend execution in those two cases (v8 breakpoint and v8 blocking), or are there are other times? We try to make guarantees that extension code will run synchronously with respect to, e.g., the web page. Also, is it guaranteed that execution will resume, synchronously, in the order in which it began?
On 2015/02/10 23:26:04, Devlin wrote: > On 2015/02/10 22:53:44, kalman wrote: > > Devlin is a better reviewer than me. > > Can you expand a bit on the CL description and the bug? In particular, does > this only suspend execution in those two cases (v8 breakpoint and v8 blocking), > or are there are other times? We try to make guarantees that extension code > will run synchronously with respect to, e.g., the web page. Also, is it > guaranteed that execution will resume, synchronously, in the order in which it > began? I've added more CL description. There are two cases: v8 breakpoint and nested event loop. If v8 is not paused and there is no running nested event loop then we execute script synchronously. And yes, there is guaranty that first executed script will be executed first after V8 resuming.
On 2015/02/11 09:04:13, kozyatinskiy wrote: > On 2015/02/10 23:26:04, Devlin wrote: > > On 2015/02/10 22:53:44, kalman wrote: > > > Devlin is a better reviewer than me. > > > > Can you expand a bit on the CL description and the bug? In particular, does > > this only suspend execution in those two cases (v8 breakpoint and v8 > blocking), > > or are there are other times? We try to make guarantees that extension code > > will run synchronously with respect to, e.g., the web page. Also, is it > > guaranteed that execution will resume, synchronously, in the order in which it > > began? > > I've added more CL description. There are two cases: v8 breakpoint and nested > event loop. If v8 is not paused and there is no running nested event loop then > we execute script synchronously. And yes, there is guaranty that first executed > script will be executed first after V8 resuming. Nested event loop can be runned by model dialog (alert, confirm, ..) or by devtools on breakpoint pause.
On 2015/02/11 10:28:46, kozyatinskiy wrote: > On 2015/02/11 09:04:13, kozyatinskiy wrote: > > On 2015/02/10 23:26:04, Devlin wrote: > > > On 2015/02/10 22:53:44, kalman wrote: > > > > Devlin is a better reviewer than me. > > > > > > Can you expand a bit on the CL description and the bug? In particular, does > > > this only suspend execution in those two cases (v8 breakpoint and v8 > > blocking), > > > or are there are other times? We try to make guarantees that extension code > > > will run synchronously with respect to, e.g., the web page. Also, is it > > > guaranteed that execution will resume, synchronously, in the order in which > it > > > began? > > > > I've added more CL description. There are two cases: v8 breakpoint and nested > > event loop. If v8 is not paused and there is no running nested event loop then > > we execute script synchronously. And yes, there is guaranty that first > executed > > script will be executed first after V8 resuming. > > Nested event loop can be runned by model dialog (alert, confirm, ..) or by > devtools on breakpoint pause. modal dialog*
First off, a few bigger items. Also, is the page subject to the same restrictions (i.e., it will also be suspended during these times)? https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:295: frame_result_index_.find(frame); If we truly have guarantees of synchronicity with respect to scripts (i.e., first in is first to execute), why do we need to cache the index? Isn't it then guaranteed that first to finish should go at the front of the list? https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (left): https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo scripts_run_info; Having each injection own a ScriptsRunInfo makes these metrics mean something completely different. The goal is to have a measurement of how long it took to run all extension scripts at each stage. https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:383: if (invalidated_while_injecting_.count((*iter)->extension_id()) == 0 && I don't think it's safe to remove this - can't scripts still be invalidated, since they execute asynchronously?
On 2015/02/11 17:49:58, Devlin wrote: > First off, a few bigger items. Also, is the page subject to the same > restrictions (i.e., it will also be suspended during these times)? Yes, page also be suspended. > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > File extensions/renderer/script_injection.cc (right): > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > extensions/renderer/script_injection.cc:295: frame_result_index_.find(frame); > If we truly have guarantees of synchronicity with respect to scripts (i.e., > first in is first to execute), why do we need to cache the index? Isn't it then > guaranteed that first to finish should go at the front of the list? We have guarantees for injection order but we have no guarantees about order of injection execution in sub frames if we want to execute injection in all current frames. > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > File extensions/renderer/script_injection_manager.cc (left): > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo > scripts_run_info; > Having each injection own a ScriptsRunInfo makes these metrics mean something > completely different. The goal is to have a measurement of how long it took to > run all extension scripts at each stage. Ok, I'll fix it. > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > extensions/renderer/script_injection_manager.cc:383: if > (invalidated_while_injecting_.count((*iter)->extension_id()) == 0 && > I don't think it's safe to remove this - can't scripts still be invalidated, > since they execute asynchronously? This can be unsafe but now we have no API for stopping started executions in blink. On other side without this CL all injections are synchronous and they don't check state of V8 in blink. It seems dangerous because we have some assertions in blink code which depends on script execution in paused state. With this CL blink decides when injection will be executed.
Friendly ping.
https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (left): https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo scripts_run_info; On 2015/02/11 17:49:58, Devlin wrote: > Having each injection own a ScriptsRunInfo makes these metrics mean something > completely different. The goal is to have a measurement of how long it took to > run all extension scripts at each stage. Still waiting on this?
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:100001) has been deleted
On 2015/02/19 17:13:01, Devlin wrote: > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > File extensions/renderer/script_injection_manager.cc (left): > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo > scripts_run_info; > On 2015/02/11 17:49:58, Devlin wrote: > > Having each injection own a ScriptsRunInfo makes these metrics mean something > > completely different. The goal is to have a measurement of how long it took to > > run all extension scripts at each stage. > > Still waiting on this? I've uploaded patch with fixing it.
On 2015/02/20 16:49:25, kozyatinskiy wrote: > On 2015/02/19 17:13:01, Devlin wrote: > > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > > File extensions/renderer/script_injection_manager.cc (left): > > > > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > > extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo > > scripts_run_info; > > On 2015/02/11 17:49:58, Devlin wrote: > > > Having each injection own a ScriptsRunInfo makes these metrics mean > something > > > completely different. The goal is to have a measurement of how long it took > to > > > run all extension scripts at each stage. > > > > Still waiting on this? > > I've uploaded patch with fixing it. The test that's failing on the bots directly tests script injection, so I suspect that there's still something that needs to change here. :)
On 2015/02/20 22:21:47, Devlin wrote: > On 2015/02/20 16:49:25, kozyatinskiy wrote: > > On 2015/02/19 17:13:01, Devlin wrote: > > > > > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > > > File extensions/renderer/script_injection_manager.cc (left): > > > > > > > > > https://codereview.chromium.org/878513005/diff/80001/extensions/renderer/scri... > > > extensions/renderer/script_injection_manager.cc:372: ScriptsRunInfo > > > scripts_run_info; > > > On 2015/02/11 17:49:58, Devlin wrote: > > > > Having each injection own a ScriptsRunInfo makes these metrics mean > > something > > > > completely different. The goal is to have a measurement of how long it > took > > to > > > > run all extension scripts at each stage. > > > > > > Still waiting on this? > > > > I've uploaded patch with fixing it. > > The test that's failing on the bots directly tests script injection, so I > suspect that there's still something that needs to change here. :) Fixed. PTAL. I'll fix compile error on Win bots when I can reproduce it.
https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection.cc:289: std::map<blink::WebLocalFrame*, size_t>::iterator it = We actually make no guarantees about result order for this api. We should be somewhat logical, and put the main frame's results first, but otherwise, we can just put them in the order they finish. So we can ditch the map, and instead just do (conceptually): if (!frame->parent()) // Main frame. results.push_front(result); else results.push_back(result); https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection.h:56: bool TryToInject(UserScript::RunLocation current_location, As a result of some of these changes, I think it best to change this to an enum of InjectionResult, which could be: INJECTION_FINISHED, INJECTION_BLOCKED, INJECTION_WAITING_FOR_PERMISSION This will make the logic in ScriptInjectionSet a bit clearer. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:270: if (scripts_run_info && scripts_run_info->HasOneRef() && IsFrameValid(frame)) ref counted lifetimes are already somewhat Evil. Coupled with the fact that you have multiple spots of logic where you need to know how many refs are left, this seems like a very fragile solution to me. What's more, there are two things that a ScriptsRunInfo does: 1. Logs that content scripts are running for the activity log. 2. Logs how long injecting content scripts took. For 1., we really shouldn't delay sending the message until the script finishes - there's no need. For 2., we should probably differentiate between synchronous and asynchronous injection times. If a script injects an alert, then it could be several seconds (or longer) before it's complete, which is normally a very, very bad metric. I propose: - Notify the activity log of injections synchronously directly after they start. - Record some metrics - scriptcount, csscount, and existing time metrics - synchronously directly after they start. - Record a set of new time metrics (with, e.g., _Blocking appened to the metric name) once they all finish, if any were blocking. We should also not use a refcounted ScriptsRunInfo. Instead, have ScriptInjectionManager own the scripts run infos for each frame/location, and when an injection (asynchronously) finishes, check if there are any other injections for that frame/location pair. If not, log the info. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:349: // As above, we might have been blocked, but that means that, in the mean Isn't the point of this patch that executing scripts can no longer block and run a nested loop? How would this still be possible?
Devlin, please take a look. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection.cc:289: std::map<blink::WebLocalFrame*, size_t>::iterator it = On 2015/02/25 18:02:15, Devlin wrote: > We actually make no guarantees about result order for this api. We should be > somewhat logical, and put the main frame's results first, but otherwise, we can > just put them in the order they finish. So we can ditch the map, and instead > just do (conceptually): > > if (!frame->parent()) // Main frame. > results.push_front(result); > else > results.push_back(result); Done. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection.h:56: bool TryToInject(UserScript::RunLocation current_location, On 2015/02/25 18:02:15, Devlin wrote: > As a result of some of these changes, I think it best to change this to an enum > of InjectionResult, which could be: > INJECTION_FINISHED, > INJECTION_BLOCKED, > INJECTION_WAITING_FOR_PERMISSION > > This will make the logic in ScriptInjectionSet a bit clearer. Done. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:270: if (scripts_run_info && scripts_run_info->HasOneRef() && IsFrameValid(frame)) On 2015/02/25 18:02:15, Devlin wrote: > ref counted lifetimes are already somewhat Evil. Coupled with the fact that you > have multiple spots of logic where you need to know how many refs are left, this > seems like a very fragile solution to me. What's more, there are two things > that a ScriptsRunInfo does: > 1. Logs that content scripts are running for the activity log. > 2. Logs how long injecting content scripts took. > For 1., we really shouldn't delay sending the message until the script finishes > - there's no need. > For 2., we should probably differentiate between synchronous and asynchronous > injection times. If a script injects an alert, then it could be several seconds > (or longer) before it's complete, which is normally a very, very bad metric. > > I propose: > - Notify the activity log of injections synchronously directly after they start. > - Record some metrics - scriptcount, csscount, and existing time metrics - > synchronously directly after they start. > - Record a set of new time metrics (with, e.g., _Blocking appened to the metric > name) once they all finish, if any were blocking. > > We should also not use a refcounted ScriptsRunInfo. Instead, have > ScriptInjectionManager own the scripts run infos for each frame/location, and > when an injection (asynchronously) finishes, check if there are any other > injections for that frame/location pair. If not, log the info. I've removed ref counted Scripts Run Info and returned back old implementation. If injection runs synchronously (when blink doesn't show modal dialog and V8 isn't paused at breakpoint) then ScripsRunInfo will log information about run in old manner. If injection runs asynchronously then ScriptsRunInfo will log information about css and js count in old manner and time spent on the transfer of injection to blink without script execution time. This corresponds to the first two points of the your proposal. Is it really necessary to log time of asynchronous injections? It seems like rarely case and time of execution can be absolutely unpredictable and depends on user actions. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:349: // As above, we might have been blocked, but that means that, in the mean On 2015/02/25 18:02:15, Devlin wrote: > Isn't the point of this patch that executing scripts can no longer block and run > a nested loop? How would this still be possible? Done.
Looks much better, overall! Thanks for making those changes. Now into the nitty-gritty. :) https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:270: if (scripts_run_info && scripts_run_info->HasOneRef() && IsFrameValid(frame)) On 2015/02/27 17:32:28, kozyatinskiy wrote: > On 2015/02/25 18:02:15, Devlin wrote: > > ref counted lifetimes are already somewhat Evil. Coupled with the fact that > you > > have multiple spots of logic where you need to know how many refs are left, > this > > seems like a very fragile solution to me. What's more, there are two things > > that a ScriptsRunInfo does: > > 1. Logs that content scripts are running for the activity log. > > 2. Logs how long injecting content scripts took. > > For 1., we really shouldn't delay sending the message until the script > finishes > > - there's no need. > > For 2., we should probably differentiate between synchronous and asynchronous > > injection times. If a script injects an alert, then it could be several > seconds > > (or longer) before it's complete, which is normally a very, very bad metric. > > > > I propose: > > - Notify the activity log of injections synchronously directly after they > start. > > - Record some metrics - scriptcount, csscount, and existing time metrics - > > synchronously directly after they start. > > - Record a set of new time metrics (with, e.g., _Blocking appened to the > metric > > name) once they all finish, if any were blocking. > > > > We should also not use a refcounted ScriptsRunInfo. Instead, have > > ScriptInjectionManager own the scripts run infos for each frame/location, and > > when an injection (asynchronously) finishes, check if there are any other > > injections for that frame/location pair. If not, log the info. > > I've removed ref counted Scripts Run Info and returned back old implementation. > If injection runs synchronously (when blink doesn't show modal dialog and V8 > isn't paused at breakpoint) then ScripsRunInfo will log information about run in > old manner. > If injection runs asynchronously then ScriptsRunInfo will log information about > css and js count in old manner and time spent on the transfer of injection to > blink without script execution time. > This corresponds to the first two points of the your proposal. > Is it really necessary to log time of asynchronous injections? It seems like > rarely case and time of execution can be absolutely unpredictable and depends on > user actions. Fair enough. More than anything else, I'd want to know how often exactly this is happening (as you point out, it should be pretty rare). Could we omit the timing logs, but insert a metric for scripts that were delayed (akin to num scripts injected)? https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:246: TryToFinish(); This TryToFinish seems wrong, since it will notify the ScriptInjectionManager. Instead, I think this should just say: if (complete_) injector_->OnInjectionComplete(...) https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:252: running_frames_++; nit: chrome style prefers pre-increment. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:262: new ScriptInjectionCallback(this, frame)); nit: indentation off. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:309: // if current frame is main frame in injection This comment isn't really helpful (it just describes the if-statement). Instead, let's put a comment like: // We guarantee that the main frame's result is at the first index, but // any sub frames results do not have guaranteed order. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:313: : base::Value::CreateNullValue()); instead of having this result.get() ? logic twice, above the if-statement for the frames, do: if (!result) result.reset(new base::Value::CreateNullValue()); and then when appending you can just do result.release(). Also, let's take out the frame if, and just do: execution_results_->Insert( frame == web_frame_ ? 0 : execution_results_->GetSize(), result.get()); https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:323: ScriptInjectionManager* manager){ nit: indenation off. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:56: // INJECTION_BLOCKED if injection is runned asynchronously and has not s/runned/running https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:88: // otherwise INJECTION_BLOCKED nit: end comments in a period. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:98: // OnInjectionComplete will be called on injector Maybe instead: "Checks if all scripts have been injected and finished." https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:131: unsigned running_frames_; use either int or size_t. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.cc:15: , web_frame_(web_frame) { comma on the previous line. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.cc:23: injection_->OnJSInjectionCompleted(web_frame_, result); indentation off. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:19: struct ScriptsRunInfo; not needed. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:21: class ScriptInjectionCallback : public blink::WebScriptExecutionCallback { Add class comment. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:23: nit: remove this line. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:27: virtual ~ScriptInjectionCallback(); ~ScriptInjectionCallback() override; https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:29: virtual void completed( no virtual needed. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:30: const blink::WebVector<v8::Local<v8::Value> >&) override; indentation off. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:33: Remove this line. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:34: ScriptInjection* injection_; Comment on lifetime. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:260: for (it = running_injections_.begin(); it != running_injections_.end(); ++it){ This looks like it clears all running injections? https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:341: // We create a separate vector for these because there is a chance that This is also not true, right? https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:364: for (ScopedVector<ScriptInjection>::iterator iter = frame_injections.begin(); Let's simplify this to: std::vector<ScriptInjection*> released_injections; frame_injections.release(&released_injections); for (ScriptInjection* injection : released_injections) TryToInject(make_scoped_ptr(injection), run_location, &scripts_run_info); released_injections.clear(); https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:383: remove this line. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:395: switch (res) { inline res. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:437: UserScript::RunLocation run_location = (iter == frame_statuses_.end() ? Why not keep run_location inlined? https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:468: false); I think it was a bug that we didn't store waiting scripts here before. We should. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injector.h:80: virtual void GetRunInfo( I think I slightly prefer having the one OnInjectionComplete method here. If the injection was blocked (and we have no run info), we can pass in nullptr (and add a check in UserScriptInjector).
Thank you for great review! :) New version uploaded. Please take a look. https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:270: if (scripts_run_info && scripts_run_info->HasOneRef() && IsFrameValid(frame)) On 2015/02/27 18:25:32, Devlin wrote: > On 2015/02/27 17:32:28, kozyatinskiy wrote: > > On 2015/02/25 18:02:15, Devlin wrote: > > > ref counted lifetimes are already somewhat Evil. Coupled with the fact that > > you > > > have multiple spots of logic where you need to know how many refs are left, > > this > > > seems like a very fragile solution to me. What's more, there are two things > > > that a ScriptsRunInfo does: > > > 1. Logs that content scripts are running for the activity log. > > > 2. Logs how long injecting content scripts took. > > > For 1., we really shouldn't delay sending the message until the script > > finishes > > > - there's no need. > > > For 2., we should probably differentiate between synchronous and > asynchronous > > > injection times. If a script injects an alert, then it could be several > > seconds > > > (or longer) before it's complete, which is normally a very, very bad metric. > > > > > > I propose: > > > - Notify the activity log of injections synchronously directly after they > > start. > > > - Record some metrics - scriptcount, csscount, and existing time metrics - > > > synchronously directly after they start. > > > - Record a set of new time metrics (with, e.g., _Blocking appened to the > > metric > > > name) once they all finish, if any were blocking. > > > > > > We should also not use a refcounted ScriptsRunInfo. Instead, have > > > ScriptInjectionManager own the scripts run infos for each frame/location, > and > > > when an injection (asynchronously) finishes, check if there are any other > > > injections for that frame/location pair. If not, log the info. > > > > I've removed ref counted Scripts Run Info and returned back old > implementation. > > If injection runs synchronously (when blink doesn't show modal dialog and V8 > > isn't paused at breakpoint) then ScripsRunInfo will log information about run > in > > old manner. > > If injection runs asynchronously then ScriptsRunInfo will log information > about > > css and js count in old manner and time spent on the transfer of injection to > > blink without script execution time. > > This corresponds to the first two points of the your proposal. > > Is it really necessary to log time of asynchronous injections? It seems like > > rarely case and time of execution can be absolutely unpredictable and depends > on > > user actions. > > Fair enough. More than anything else, I'd want to know how often exactly this > is happening (as you point out, it should be pretty rare). Could we omit the > timing logs, but insert a metric for scripts that were delayed (akin to num > scripts injected)? I've added _BlockedScriptCount metrics to ScriptsRunInfo and added these metrics to histograms.xml with you and Kalman as owners. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:246: TryToFinish(); On 2015/02/27 18:25:32, Devlin wrote: > This TryToFinish seems wrong, since it will notify the ScriptInjectionManager. > Instead, I think this should just say: > if (complete_) > injector_->OnInjectionComplete(...) If we have synchronous injections in two frames then first injection will call TryToFinish in OnJSInjectionCompleted. This first call will change nothing because running_frames = 0 but all_injection_started is false. Then we start second injection, it call TryToFinish and change nothing too with the same reason. And we need to call TryToFinish one more time in this case. We can change complete_ only in TryToFinish methods and I suppose we can't just check if(complete_) without TryToFinish call. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:252: running_frames_++; On 2015/02/27 18:25:32, Devlin wrote: > nit: chrome style prefers pre-increment. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:262: new ScriptInjectionCallback(this, frame)); On 2015/02/27 18:25:32, Devlin wrote: > nit: indentation off. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:309: // if current frame is main frame in injection On 2015/02/27 18:25:32, Devlin wrote: > This comment isn't really helpful (it just describes the if-statement). > Instead, let's put a comment like: > // We guarantee that the main frame's result is at the first index, but > // any sub frames results do not have guaranteed order. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:313: : base::Value::CreateNullValue()); On 2015/02/27 18:25:32, Devlin wrote: > instead of having this result.get() ? logic twice, above the if-statement for > the frames, do: > if (!result) > result.reset(new base::Value::CreateNullValue()); > > and then when appending you can just do result.release(). > > Also, let's take out the frame if, and just do: > execution_results_->Insert( > frame == web_frame_ ? 0 : execution_results_->GetSize(), result.get()); Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.cc:323: ScriptInjectionManager* manager){ On 2015/02/27 18:25:32, Devlin wrote: > nit: indenation off. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:56: // INJECTION_BLOCKED if injection is runned asynchronously and has not On 2015/02/27 18:25:32, Devlin wrote: > s/runned/running Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:88: // otherwise INJECTION_BLOCKED On 2015/02/27 18:25:32, Devlin wrote: > nit: end comments in a period. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:98: // OnInjectionComplete will be called on injector On 2015/02/27 18:25:32, Devlin wrote: > Maybe instead: "Checks if all scripts have been injected and finished." Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection.h:131: unsigned running_frames_; On 2015/02/27 18:25:32, Devlin wrote: > use either int or size_t. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.cc:15: , web_frame_(web_frame) { On 2015/02/27 18:25:32, Devlin wrote: > comma on the previous line. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.cc:23: injection_->OnJSInjectionCompleted(web_frame_, result); On 2015/02/27 18:25:32, Devlin wrote: > indentation off. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:19: struct ScriptsRunInfo; On 2015/02/27 18:25:33, Devlin wrote: > not needed. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:21: class ScriptInjectionCallback : public blink::WebScriptExecutionCallback { On 2015/02/27 18:25:32, Devlin wrote: > Add class comment. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:23: On 2015/02/27 18:25:32, Devlin wrote: > nit: remove this line. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:27: virtual ~ScriptInjectionCallback(); On 2015/02/27 18:25:33, Devlin wrote: > ~ScriptInjectionCallback() override; Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:29: virtual void completed( On 2015/02/27 18:25:33, Devlin wrote: > no virtual needed. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:30: const blink::WebVector<v8::Local<v8::Value> >&) override; On 2015/02/27 18:25:32, Devlin wrote: > indentation off. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:33: On 2015/02/27 18:25:32, Devlin wrote: > Remove this line. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:34: ScriptInjection* injection_; On 2015/02/27 18:25:32, Devlin wrote: > Comment on lifetime. Added to class comment. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:260: for (it = running_injections_.begin(); it != running_injections_.end(); ++it){ On 2015/02/27 18:25:33, Devlin wrote: > This looks like it clears all running injections? Fixed. It always clears first injection without fix. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:341: // We create a separate vector for these because there is a chance that On 2015/02/27 18:25:33, Devlin wrote: > This is also not true, right? I removed comment about nested message loop. But we need to have here separate vector otherwise we need to store new pending injections in separate vector and add to pending_injections_ after inject cycle. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:364: for (ScopedVector<ScriptInjection>::iterator iter = frame_injections.begin(); On 2015/02/27 18:25:33, Devlin wrote: > Let's simplify this to: > std::vector<ScriptInjection*> released_injections; > frame_injections.release(&released_injections); > for (ScriptInjection* injection : released_injections) > TryToInject(make_scoped_ptr(injection), run_location, &scripts_run_info); > released_injections.clear(); Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:383: On 2015/02/27 18:25:33, Devlin wrote: > remove this line. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:395: switch (res) { On 2015/02/27 18:25:33, Devlin wrote: > inline res. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:437: UserScript::RunLocation run_location = (iter == frame_statuses_.end() ? On 2015/02/27 18:25:33, Devlin wrote: > Why not keep run_location inlined? Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:468: false); On 2015/02/27 18:25:33, Devlin wrote: > I think it was a bug that we didn't store waiting scripts here before. We > should. Done. https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/878513005/diff/160001/extensions/renderer/scr... extensions/renderer/script_injector.h:80: virtual void GetRunInfo( On 2015/02/27 18:25:33, Devlin wrote: > I think I slightly prefer having the one OnInjectionComplete method here. If > the injection was blocked (and we have no run info), we can pass in nullptr (and > add a check in UserScriptInjector). If we want to get run info for sync and async (without elapsed time) injection, we need to divide this method because I need to receive information about run somewhere in ScriptInjection::TryToInject for both type of injections.
https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.cc:264: new ScriptInjectionCallback(this, frame)); Is it at all possible for the callback to outlive the script injection? Presumably not, if the callback is tied to the renderer, but figured it's worth asking. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.cc:335: script_injection_manager_->OnInjectionFinished(this); I still kinda dislike that this notifies the manager in all cases. Let's pass in a bool, should_notify. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:80: void SetScriptInjectionManager(ScriptInjectionManager* manager); Instead, let's just pass in the ScriptInjectionManager to TryToInject(). https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:135: // Flag is true when injections for each frame started nit: comments need to end in a period. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:136: bool all_injection_started_; nit: all_injections_started_ (plural) https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:29: void completed(const blink::WebVector<v8::Local<v8::Value> >&) override; nit: chrome style requires named parameters. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:260: for (it = running_injections_.begin(); it != running_injections_.end(); ++it){ Prefer: ScopedVector<ScriptInjection>::iterator iter = running_injections_.find(injection); if (iter != running_injections_.end()) running_injections_.erase(iter); https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:475: injection->SetScriptInjectionManager(this); Shouldn't this already be set? (We can't have an injection waiting for permission if it hasn't tried to inject) https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:482: if (res == ScriptInjection::INJECTION_BLOCKED) { nit: no brackets on single-line ifs. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.h:48: // Notifies that an injection has been finished nit: needs a period (same for below) https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injector.h:79: // Fill scriptrs run info nit: fix this comment. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:38: UMA_HISTOGRAM_COUNTS_100("Extensions.InjectStart_BlockedScriptCount", Here, too, Blocking, not blocked. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/scripts_run_info.h:36: size_t num_blocked_js; nit: "blocked" has a slightly different connotation than "blocking", which is I think what we want here.
https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.cc:264: new ScriptInjectionCallback(this, frame)); On 2015/03/03 00:34:10, Devlin wrote: > Is it at all possible for the callback to outlive the script injection? > Presumably not, if the callback is tied to the renderer, but figured it's worth > asking. If we destroy injection before receiving callback, then callback can outlive injection. In current implementation it is impossible. ScriptInjection in destructor can call method like OnInjectionDestroyed on each active callback, but it seems not necessary. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.cc:335: script_injection_manager_->OnInjectionFinished(this); On 2015/03/03 00:34:10, Devlin wrote: > I still kinda dislike that this notifies the manager in all cases. Let's pass > in a bool, should_notify. I've used script_injection_manager_ != NULL check instead. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:80: void SetScriptInjectionManager(ScriptInjectionManager* manager); On 2015/03/03 00:34:10, Devlin wrote: > Instead, let's just pass in the ScriptInjectionManager to TryToInject(). Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:135: // Flag is true when injections for each frame started On 2015/03/03 00:34:10, Devlin wrote: > nit: comments need to end in a period. Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection.h:136: bool all_injection_started_; On 2015/03/03 00:34:10, Devlin wrote: > nit: all_injections_started_ (plural) Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:29: void completed(const blink::WebVector<v8::Local<v8::Value> >&) override; On 2015/03/03 00:34:10, Devlin wrote: > nit: chrome style requires named parameters. Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:260: for (it = running_injections_.begin(); it != running_injections_.end(); ++it){ On 2015/03/03 00:34:10, Devlin wrote: > Prefer: > ScopedVector<ScriptInjection>::iterator iter = > running_injections_.find(injection); > if (iter != running_injections_.end()) > running_injections_.erase(iter); Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:475: injection->SetScriptInjectionManager(this); On 2015/03/03 00:34:10, Devlin wrote: > Shouldn't this already be set? (We can't have an injection waiting for > permission if it hasn't tried to inject) Removed. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:482: if (res == ScriptInjection::INJECTION_BLOCKED) { On 2015/03/03 00:34:10, Devlin wrote: > nit: no brackets on single-line ifs. Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injection_manager.h:48: // Notifies that an injection has been finished On 2015/03/03 00:34:10, Devlin wrote: > nit: needs a period (same for below) Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/script_injector.h:79: // Fill scriptrs run info On 2015/03/03 00:34:10, Devlin wrote: > nit: fix this comment. Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:38: UMA_HISTOGRAM_COUNTS_100("Extensions.InjectStart_BlockedScriptCount", On 2015/03/03 00:34:10, Devlin wrote: > Here, too, Blocking, not blocked. Done. https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/878513005/diff/180001/extensions/renderer/scr... extensions/renderer/scripts_run_info.h:36: size_t num_blocked_js; On 2015/03/03 00:34:10, Devlin wrote: > nit: "blocked" has a slightly different connotation than "blocking", which is I > think what we want here. Done.
https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.h:52: nit: no newline here (group override methods from the same super class). https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:163: return Inject(injection_host, scripts_run_info); Let's instead do: InjectionResult result = Inject(...); // If the injection is blocked, we need to set the manager so we can // notify it upon completion. if (result == INJECTION_BLOCKED) script_injection_manager_ = manager; https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:332: script_injection_manager_->OnInjectionFinished(this); This will still notify the manager in all cases, since script_injection_manager_ is set at the top of TryToInject(). See above. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:73: void OnJSInjectionCompleted(blink::WebLocalFrame* frame, Nit: In this file, we have the style of "Js" in method names, so please make this "OnJsInjectionCompleted" https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:131: // Results storage nit: end in a period. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:20: // A script injection callback which contains completed method Let's instead say: // A wrapper around a callback to notify a script injection when injection completes. // This class manages its own lifetime.
https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.h:52: On 2015/03/03 23:47:02, Devlin wrote: > nit: no newline here (group override methods from the same super class). Done. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:163: return Inject(injection_host, scripts_run_info); On 2015/03/03 23:47:03, Devlin wrote: > Let's instead do: > InjectionResult result = Inject(...); > // If the injection is blocked, we need to set the manager so we can > // notify it upon completion. > if (result == INJECTION_BLOCKED) > script_injection_manager_ = manager; Done. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:332: script_injection_manager_->OnInjectionFinished(this); On 2015/03/03 23:47:03, Devlin wrote: > This will still notify the manager in all cases, since script_injection_manager_ > is set at the top of TryToInject(). See above. Done. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:73: void OnJSInjectionCompleted(blink::WebLocalFrame* frame, On 2015/03/03 23:47:03, Devlin wrote: > Nit: In this file, we have the style of "Js" in method names, so please make > this "OnJsInjectionCompleted" Done. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:131: // Results storage On 2015/03/03 23:47:03, Devlin wrote: > nit: end in a period. Done. https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection_callback.h (right): https://codereview.chromium.org/878513005/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection_callback.h:20: // A script injection callback which contains completed method On 2015/03/03 23:47:03, Devlin wrote: > Let's instead say: > // A wrapper around a callback to notify a script injection when injection > completes. > // This class manages its own lifetime. Done.
lgtm with one final nit. :) https://codereview.chromium.org/878513005/diff/220001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/220001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:41: if ((num_css || num_js) && !num_blocking_js) nit: make this else if (num_css || num_js) (Same goes for below)
kozyatinskiy@chromium.org changed reviewers: + asvitkine@chromium.org
@asvitkine, please take a look. I need owner lgtm for histograms.xml. https://codereview.chromium.org/878513005/diff/220001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/220001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:41: if ((num_css || num_js) && !num_blocking_js) On 2015/03/04 18:09:31, Devlin wrote: > nit: make this > else if (num_css || num_js) > > (Same goes for below) Done.
asvitkine@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow, can you do the initial review for histograms in this CL? Thanks.
A few more last things (a few of which may have come from merge conflicts). https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:60: // NOTE: |injection_host| may be NULL, if the injection_host is removed! Remove |injection_host| comment. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:67: // returns INJECTION_BLOCKED if injection is runned asynchronously. nit: "if the injection is ran asynchronously" (not runned) https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:75: const blink::WebVector<v8::Local<v8::Value> >&); nit: assign a name to the vector (e.g., result) https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:381: pending_injections_.push_back(injection.release()); need a break here. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:40: } else if (num_css || num_js) if an "else" has brackets on one side, it should on the other, too. So this should be: if (num_blocking_js) { ... } else if (num_css || num_js) { ... } (Same for below.)
https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:60: // NOTE: |injection_host| may be NULL, if the injection_host is removed! On 2015/03/05 16:32:13, Devlin wrote: > Remove |injection_host| comment. Done. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:67: // returns INJECTION_BLOCKED if injection is runned asynchronously. On 2015/03/05 16:32:12, Devlin wrote: > nit: "if the injection is ran asynchronously" (not runned) Done. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection.h:75: const blink::WebVector<v8::Local<v8::Value> >&); On 2015/03/05 16:32:13, Devlin wrote: > nit: assign a name to the vector (e.g., result) Done. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:381: pending_injections_.push_back(injection.release()); On 2015/03/05 16:32:13, Devlin wrote: > need a break here. Done. https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/878513005/diff/240001/extensions/renderer/scr... extensions/renderer/scripts_run_info.cc:40: } else if (num_css || num_js) On 2015/03/05 16:32:13, Devlin wrote: > if an "else" has brackets on one side, it should on the other, too. So this > should be: > if (num_blocking_js) { > ... > } else if (num_css || num_js) { > ... > } > > (Same for below.) Done.
lgtm https://codereview.chromium.org/878513005/diff/260001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/878513005/diff/260001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8976: </histogram> I will note that you can make this a bit more concise by using <histogram_suffixes name=".." ordering="prefix"> And put InjectIdle InjectStart InjectEnd as the suffixes. To me this feels short enough that it seems fine. But it is something to consider for cases like this.
will still need asvitkine@ to review.
lgtm
https://codereview.chromium.org/878513005/diff/260001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/878513005/diff/260001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8976: </histogram> On 2015/03/05 22:38:23, rkaplow wrote: > I will note that you can make this a bit more concise by using > <histogram_suffixes name=".." ordering="prefix"> > And put > InjectIdle > InjectStart > InjectEnd > as the suffixes. > > To me this feels short enough that it seems fine. But it is something to > consider for cases like this. Acknowledged.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/878513005/#ps260001 (title: " ")
The CQ bit was unchecked by kozyatinskiy@chromium.org
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878513005/260001
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/c8bc9a58dc08a98fe889c269b680ead30558d372 Cr-Commit-Position: refs/heads/master@{#319425} |
