Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2733093002:
Set an initial background for RenderFrameHosts during commit. (Closed)
DescriptionSet an initial background for RenderFrameHosts during commit.
This was originally implemented in https://codereview.chromium.org/2466413009,
but was broken by https://codereview.chromium.org/2496233003. Now re-adding
with more explicit code, plus a unittest.
BUG=470669
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2733093002
Cr-Commit-Position: refs/heads/master@{#455612}
Committed: https://chromium.googlesource.com/chromium/src/+/5f3c1abd7dc2df3024315ec66eb46e89a5e146c8
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #Patch Set 5 : none #Patch Set 6 : none #
Total comments: 5
Patch Set 7 : none #Patch Set 8 : none #
Messages
Total messages: 29 (22 generated)
Description was changed from ========== none BUG= ========== to ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't delete the RenderWidgetHostView. BUG=470669 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by chrishtr@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Don't delete the RenderWidgetHostView. BUG=470669 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Set an initial background for RenderFrameHosts during commit. This was originally implemented in https://codereview.chromium.org/2466413009, but was broken by https://codereview.chromium.org/2496233003. Now re-adding with more explicit code, plus a unittest. BUG=470669 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
chrishtr@chromium.org changed reviewers: + nasko@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by chrishtr@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2175: if (old_render_frame_host && old_render_frame_host->GetView()) { Why null check old_render_frame_host? It is set unconditionally above and used without checking for nullness further down. https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2257: delegate_->NotifyInitialBackground(old_background_color, Is there any specific reason to go through the delegate interface? This class manages both the old and the new RFH, so it can just tell the new RFH directly what the background color is. https://codereview.chromium.org/2733093002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2733093002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4098: if (new_view) { nit: No need for {} on one line if statements.
https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2175: if (old_render_frame_host && old_render_frame_host->GetView()) { On 2017/03/08 at 22:54:20, nasko (slow) wrote: > Why null check old_render_frame_host? It is set unconditionally above and used without checking for nullness further down. Done. https://codereview.chromium.org/2733093002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2257: delegate_->NotifyInitialBackground(old_background_color, On 2017/03/08 at 22:54:20, nasko (slow) wrote: > Is there any specific reason to go through the delegate interface? This class manages both the old and the new RFH, so it can just tell the new RFH directly what the background color is. Just thought it might be cleaner. I'm happy to inline if you think that;s better.
Simplified to avoid the delegate interface.
The CQ bit was checked by chrishtr@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...
LGTM
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@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": 140001, "attempt_start_ts": 1489015135303520,
"parent_rev": "3854543fdcea4ffd600b2a0fbef5ef59c4ba1388", "commit_rev":
"5f3c1abd7dc2df3024315ec66eb46e89a5e146c8"}
Message was sent while issue was closed.
Description was changed from ========== Set an initial background for RenderFrameHosts during commit. This was originally implemented in https://codereview.chromium.org/2466413009, but was broken by https://codereview.chromium.org/2496233003. Now re-adding with more explicit code, plus a unittest. BUG=470669 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Set an initial background for RenderFrameHosts during commit. This was originally implemented in https://codereview.chromium.org/2466413009, but was broken by https://codereview.chromium.org/2496233003. Now re-adding with more explicit code, plus a unittest. BUG=470669 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2733093002 Cr-Commit-Position: refs/heads/master@{#455612} Committed: https://chromium.googlesource.com/chromium/src/+/5f3c1abd7dc2df3024315ec66eb4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5f3c1abd7dc2df3024315ec66eb4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
