Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677873002:
Convert ExtensionWebContentsObserver to use the new navigation callbacks. (Closed)
DescriptionConvert ExtensionWebContentsObserver to use the new navigation callbacks.
BUG=682002
Review-Url: https://codereview.chromium.org/2677873002
Cr-Commit-Position: refs/heads/master@{#448336}
Committed: https://chromium.googlesource.com/chromium/src/+/6987a2dae3753b2adf9942342bc4b30faf206a39
Patch Set 1 #
Total comments: 4
Patch Set 2 : review comments #
Messages
Total messages: 24 (18 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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Convert ExtensionWebContentsObserver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ExtensionWebContentsObserver to use the new navigation callbacks. BUG=682002 ==========
jam@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2677873002/diff/20001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2677873002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:154: pm->DidNavigateRenderFrameHost(render_frame_host); This should only be called if |frame_extension| is valid, right? (The original code would early-exit on line 171) https://codereview.chromium.org/2677873002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:159: } else if (!navigation_handle->IsSamePage() && frame_extension) { Hmm... this is a match with previous behavior, but the fact that we unconditionally unregister the frame but conditionally register it is a little weird. My guess is that the details.is_in_page check before (in DidNavigateAnyFrame) was because that's cheaper than GetExtensionFromFrame() and IsRenderFrameHostRegistered(). If the frame isn't registered at this point, it probably should be. I'd suggest we take out the IsSamePage() check here, but if you wanted to summarize it in a TODO(devlin), we can address it in a followup.
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/2677873002/diff/20001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2677873002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:154: pm->DidNavigateRenderFrameHost(render_frame_host); On 2017/02/06 17:15:27, Devlin wrote: > This should only be called if |frame_extension| is valid, right? (The original > code would early-exit on line 171) Sure. I didn't know if it was possible to reach here with a null frame_extension in practice, so I figured I'd send a cl to see (as opposed to the section below, where we do need it). Added a check. https://codereview.chromium.org/2677873002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:159: } else if (!navigation_handle->IsSamePage() && frame_extension) { On 2017/02/06 17:15:27, Devlin wrote: > Hmm... this is a match with previous behavior, but the fact that we > unconditionally unregister the frame but conditionally register it is a little > weird. My guess is that the details.is_in_page check before (in > DidNavigateAnyFrame) was because that's cheaper than GetExtensionFromFrame() and > IsRenderFrameHostRegistered(). If the frame isn't registered at this point, it > probably should be. > > I'd suggest we take out the IsSamePage() check here, but if you wanted to > summarize it in a TODO(devlin), we can address it in a followup. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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": 1486407799387870,
"parent_rev": "61f3e5aa8bad5412567e86469feeff90ece011ea", "commit_rev":
"6987a2dae3753b2adf9942342bc4b30faf206a39"}
Message was sent while issue was closed.
Description was changed from ========== Convert ExtensionWebContentsObserver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ExtensionWebContentsObserver to use the new navigation callbacks. BUG=682002 Review-Url: https://codereview.chromium.org/2677873002 Cr-Commit-Position: refs/heads/master@{#448336} Committed: https://chromium.googlesource.com/chromium/src/+/6987a2dae3753b2adf9942342bc4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6987a2dae3753b2adf9942342bc4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
