Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797423002:
Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed (Closed)
|
Created:
3 years, 8 months ago by Kunihiko Sakamoto Modified:
3 years, 8 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not log Extensions.InjectedScriptExecutionTime histograms if script not executed
If a script injection is cancelled (e.g. context destroyed before
execution), ScriptInjectionCallback::willExecute() is not called but
::completed() is called. In such cases, execution time shouldn't be
reported.
BUG=708897
Review-Url: https://codereview.chromium.org/2797423002
Cr-Commit-Position: refs/heads/master@{#462746}
Committed: https://chromium.googlesource.com/chromium/src/+/4178f50345634d8ff3c1dfb38ea84f490f5b7132
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments addressed #
Messages
Total messages: 19 (14 generated)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP BUG=708897 ========== to ========== Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed If a script injection is cancelled (e.g. context destroyed before execution), ScriptInjectionCallback::willExecute() is not called but ::completed() is called. In such cases, execution time shouldn't be logged. BUG=708897 ==========
Description was changed from ========== Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed If a script injection is cancelled (e.g. context destroyed before execution), ScriptInjectionCallback::willExecute() is not called but ::completed() is called. In such cases, execution time shouldn't be logged. BUG=708897 ========== to ========== Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed If a script injection is cancelled (e.g. context destroyed before execution), ScriptInjectionCallback::willExecute() is not called but ::completed() is called. In such cases, execution time shouldn't be reported. BUG=708897 ==========
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
ksakamoto@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
lgtm % nits https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:95: if (!start_time_.is_null()) Can we document this, either here or in OnJsInjectionCompleted? // If the script will never execute (such as if the context is // destroyed), willExecute() will not be called, but OnCompleted() // will. Only log a time for execution if the script, in fact, // executed. https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:329: elapsed.has_value()) { nit: base::Optional has an operator bool(), so this can just be && elapsed https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:331: elapsed.value()); optional nit: I'd slightly prefer *elapsed here (and below), since it's a bit shorter and more conventional, but I don't have a strong preference.
Thanks for the review! https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:95: if (!start_time_.is_null()) On 2017/04/06 21:32:08, Devlin wrote: > Can we document this, either here or in OnJsInjectionCompleted? > > // If the script will never execute (such as if the context is > // destroyed), willExecute() will not be called, but OnCompleted() > // will. Only log a time for execution if the script, in fact, > // executed. Done. https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:329: elapsed.has_value()) { On 2017/04/06 21:32:08, Devlin wrote: > nit: base::Optional has an operator bool(), so this can just be > && elapsed Done. https://codereview.chromium.org/2797423002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection.cc:331: elapsed.value()); On 2017/04/06 21:32:08, Devlin wrote: > optional nit: I'd slightly prefer *elapsed here (and below), since it's a bit > shorter and more conventional, but I don't have a strong preference. Done.
The CQ bit was checked by ksakamoto@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/2797423002/#ps40001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491529973999770,
"parent_rev": "5a918a3454582514556a66d18bdf402e4488381e", "commit_rev":
"4178f50345634d8ff3c1dfb38ea84f490f5b7132"}
Message was sent while issue was closed.
Description was changed from ========== Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed If a script injection is cancelled (e.g. context destroyed before execution), ScriptInjectionCallback::willExecute() is not called but ::completed() is called. In such cases, execution time shouldn't be reported. BUG=708897 ========== to ========== Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed If a script injection is cancelled (e.g. context destroyed before execution), ScriptInjectionCallback::willExecute() is not called but ::completed() is called. In such cases, execution time shouldn't be reported. BUG=708897 Review-Url: https://codereview.chromium.org/2797423002 Cr-Commit-Position: refs/heads/master@{#462746} Committed: https://chromium.googlesource.com/chromium/src/+/4178f50345634d8ff3c1dfb38ea8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4178f50345634d8ff3c1dfb38ea8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
