Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656483004:
Convert ExtensionWebUiTimer to use the new navigation callbacks. (Closed)
DescriptionConvert ExtensionWebUiTimer to use the new navigation callbacks.
BUG=682002
Review-Url: https://codereview.chromium.org/2656483004
Cr-Commit-Position: refs/heads/master@{#445897}
Committed: https://chromium.googlesource.com/chromium/src/+/3a5fa84704f2917edc43a1f4c5a827b65e25d867
Patch Set 1 #Patch Set 2 : fix plznavigate case #
Total comments: 7
Patch Set 3 : fix comment #Messages
Total messages: 23 (17 generated)
The CQ bit was checked by jam@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Description was changed from ========== Convert ExtensionWebUiTimer to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ExtensionWebUiTimer to use the new navigation callbacks. BUG=682002 ==========
jam@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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.
A few quick questions https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (left): https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:75: // *This appears to be for in-page navigations like hash changes. Is this part no longer true? https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:77: // will received this current callback. nit: will have received? will still receive? https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:81: UMA_HISTOGRAM_TIMES("Extensions.WebUi.LoadCompletedInMainFrame.MD", Are DidStartNavigation() and DidStartProvisionalLoadForFrame() called at almost exactly the same point? Or will we see a potential difference here because of this change?
The CQ bit was checked by jam@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...
https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (left): https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:75: // *This appears to be for in-page navigations like hash changes. On 2017/01/24 15:59:06, Devlin (catching up) wrote: > Is this part no longer true? I'm not sure why it had to have this originally. I looked and this landed only 4 months ago, so maybe it was oopif related that led to this class being created for frames? Anyways, with PlzNavigate, this class is constructed for the subframe of this webui page. When that happens, the second instance won't get a DidStartNavigation for the main frame (that already happened in the first instance). However both instances will see a DocumentOnLoadCompletedInMainFrame when the webui page finishes loading. https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:77: // will received this current callback. On 2017/01/24 15:59:06, Devlin (catching up) wrote: > nit: will have received? will still receive? Done. https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:81: UMA_HISTOGRAM_TIMES("Extensions.WebUi.LoadCompletedInMainFrame.MD", On 2017/01/24 15:59:06, Devlin (catching up) wrote: > Are DidStartNavigation() and DidStartProvisionalLoadForFrame() called at almost > exactly the same point? Or will we see a potential difference here because of > this change? For non-PlzNavigate, the timing will be pretty much the same (the same IPC handler that calls DidStartProvisionalLoadForFrame ends up calling DidStartNavigation right after). For PlzNavigate, with all things navigation related the order/timing is different. DidStartNavigation will be called right when the WebContents is navigated, but DidStartProvisionalLoadForFrame is only when we get the notification from the renderer. The are probably a bunch of histograms which will look quite different before/after PlzNavigate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/2656483004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:81: UMA_HISTOGRAM_TIMES("Extensions.WebUi.LoadCompletedInMainFrame.MD", On 2017/01/24 16:52:20, jam wrote: > On 2017/01/24 15:59:06, Devlin (catching up) wrote: > > Are DidStartNavigation() and DidStartProvisionalLoadForFrame() called at > almost > > exactly the same point? Or will we see a potential difference here because of > > this change? > > For non-PlzNavigate, the timing will be pretty much the same (the same IPC > handler that calls DidStartProvisionalLoadForFrame ends up calling > DidStartNavigation right after). > > For PlzNavigate, with all things navigation related the order/timing is > different. DidStartNavigation will be called right when the WebContents is > navigated, but DidStartProvisionalLoadForFrame is only when we get the > notification from the renderer. > > The are probably a bunch of histograms which will look quite different > before/after PlzNavigate. Yeah, I'm not worried about before/after PlzNavigate, because we'll have to take that into account and we can filter by feature. So as long as this is the same without PlzNavigate, this sounds fine.
The CQ bit was checked by jam@chromium.org
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": 1485306896395370,
"parent_rev": "0e3075995e49f524bfe3d7eadeb85bb508c83597", "commit_rev":
"3a5fa84704f2917edc43a1f4c5a827b65e25d867"}
Message was sent while issue was closed.
Description was changed from ========== Convert ExtensionWebUiTimer to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ExtensionWebUiTimer to use the new navigation callbacks. BUG=682002 Review-Url: https://codereview.chromium.org/2656483004 Cr-Commit-Position: refs/heads/master@{#445897} Committed: https://chromium.googlesource.com/chromium/src/+/3a5fa84704f2917edc43a1f4c5a8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3a5fa84704f2917edc43a1f4c5a8... |
