Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341553002:
[MD Extensions] Add some performance metrics (Closed)
Description[MD Extensions] Add some performance metrics
Add some performance metrics for the MD and non-MD extensions pages so
we can begin gathering data. To start, use the signals for
DocumentLoadedInFrame() for the main frame and
DocumentOnLoadCompletedInMainFrame().
BUG=529395
Committed: https://crrev.com/de40667ef0c752dfe98e70ed08d06a5479c348fb
Cr-Commit-Position: refs/heads/master@{#418692}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Dan's #
Total comments: 3
Messages
Total messages: 16 (6 generated)
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org, mpearson@chromium.org
Dan and Mark, mind taking a look?
lgtm https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... chrome/browser/ui/webui/extensions/extensions_ui.cc:6: nit: #include <memory> for std::unique_ptr https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... chrome/browser/ui/webui/extensions/extensions_ui.cc:52: timer_.reset(new base::ElapsedTimer()); i assume this captures reloads correctly?
Description was changed from ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 ========== to ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 ==========
/cc tsergeant@ and stevenjb@
https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... chrome/browser/ui/webui/extensions/extensions_ui.cc:6: On 2016/09/13 23:40:13, Dan Beam wrote: > nit: > > #include <memory> > > for std::unique_ptr Done. https://codereview.chromium.org/2341553002/diff/1/chrome/browser/ui/webui/ext... chrome/browser/ui/webui/extensions/extensions_ui.cc:52: timer_.reset(new base::ElapsedTimer()); On 2016/09/13 23:40:13, Dan Beam wrote: > i assume this captures reloads correctly? Yep, it seems to. FWIW, this is also what settings does, so I certainly hope so. :)
histograms.xml lgtm https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:57: void DocumentLoadedInFrame( side nit: shouldn't you have Main in the function name?
https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:57: void DocumentLoadedInFrame( On 2016/09/14 19:47:31, Mark P wrote: > side nit: shouldn't you have Main in the function name? Nope, this overrides a WebContentsObserver method. We determine it's the main frame by comparing the render_frame_host with web_contents()->GetMainFrame() (line 59 of this patch set).
silly me --mark https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2341553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:57: void DocumentLoadedInFrame( On 2016/09/14 19:50:09, Devlin wrote: > On 2016/09/14 19:47:31, Mark P wrote: > > side nit: shouldn't you have Main in the function name? > > Nope, this overrides a WebContentsObserver method. We determine it's the main > frame by comparing the render_frame_host with web_contents()->GetMainFrame() > (line 59 of this patch set). Acknowledged.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2341553002/#ps20001 (title: "Dan's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 ========== to ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 ========== to ========== [MD Extensions] Add some performance metrics Add some performance metrics for the MD and non-MD extensions pages so we can begin gathering data. To start, use the signals for DocumentLoadedInFrame() for the main frame and DocumentOnLoadCompletedInMainFrame(). BUG=529395 Committed: https://crrev.com/de40667ef0c752dfe98e70ed08d06a5479c348fb Cr-Commit-Position: refs/heads/master@{#418692} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/de40667ef0c752dfe98e70ed08d06a5479c348fb Cr-Commit-Position: refs/heads/master@{#418692} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
