Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1759323003:
Add resize gutter layers to DelegatedFrameHost (Closed)
|
Created:
4 years, 9 months ago by jbauman Modified:
4 years, 9 months ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd resize gutter layers to DelegatedFrameHost
If the current delegated frame size doesn't match the expected size of
the renderer, use the body color from the renderer to create layers
along the right and bottom sides to fill in the gap. This improves the
appearance of resizing the new tab page and other pages with light
backgrounds.
BUG=505612
Committed: https://crrev.com/3cb637dda82dc3df902f9341718a8a785368204b
Cr-Commit-Position: refs/heads/master@{#380739}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 15
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Add resize gutter layers ========== to ========== Add resize gutter layers BUG=505612 ==========
Description was changed from ========== Add resize gutter layers BUG=505612 ========== to ========== Add resize gutter layers to DelegatedFrameHost If the current delegated frame size doesn't match the expected size of the renderer, use the body color from the renderer to create layers along the right and bottom sides to fill in the gap. This improves the appearance of resizing the new tab page and other pages with light backgrounds. BUG=505612 ==========
jbauman@chromium.org changed reviewers: + danakj@chromium.org
I notice there are no tests, what can our story be for that?
On 2016/03/05 01:29:37, danakj wrote: > I notice there are no tests, what can our story be for that? Good point. I've added RenderWidgetHostViewAuraTest.DelegatedFrameGutter for this.
https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:72: background_color_(SK_ColorWHITE), There's some work to be done to not show the wrong color during navigation: https://bugs.chromium.org/p/chromium/issues/detail?id=470669 And this adds yet another WHITE constant somewhere, as well as another place that decides on the right color to use, which I'm worried will cause some weird stuff like black background white gutters sometimes or something. Can you alleviate my fears or find a way to get a shared value for this? https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:268: // The right gutter also include the bottom-right corner, if necessary. includes https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:275: client_->DelegatedFrameHostDesiredSizeInDIP().height())); nit: put this value in a temp |height| and move the comment above down onto that variable? https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:277: client_->DelegatedFrameHostGetLayer()->Add(right_gutter_.get()); One design question.. could cc::SurfaceLayer just show the background color as solid color quads gutters from its AppendQuads? Does it have the info it would need to do so? Would that be simpler or worse? https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:289: current_frame_size_in_dip_.width(), nit: put this into a |width| var too, it's nice to compute them both beside each other? https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:496: damage_rect_in_dip); When if ever should damage include the gutters? https://codereview.chromium.org/1759323003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1759323003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1694: gfx::Size view_size(100, 100); nit: large_size
https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:72: background_color_(SK_ColorWHITE), On 2016/03/08 00:09:43, danakj wrote: > There's some work to be done to not show the wrong color during navigation: > https://bugs.chromium.org/p/chromium/issues/detail?id=470669 > > And this adds yet another WHITE constant somewhere, as well as another place > that decides on the right color to use, which I'm worried will cause some weird > stuff like black background white gutters sometimes or something. Can you > alleviate my fears or find a way to get a shared value for this? Actually this initialization isn't really necessary (I just did it because I wanted to be sure we weren't reading uninitialized memory in crazy cases - I could remove it). We only create the gutters after we've received a frame from the renderer which informs us of the metadata.root_background_color. That color was originally intended to be used to create resize gutters, and we already use it for that purpose on android. It's not guaranteed to always be correct, but in general it seems pretty good. https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:268: // The right gutter also include the bottom-right corner, if necessary. On 2016/03/08 00:09:44, danakj wrote: > includes Done. https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:275: client_->DelegatedFrameHostDesiredSizeInDIP().height())); On 2016/03/08 00:09:44, danakj wrote: > nit: put this value in a temp |height| and move the comment above down onto that > variable? Done. https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:277: client_->DelegatedFrameHostGetLayer()->Add(right_gutter_.get()); On 2016/03/08 00:09:43, danakj wrote: > One design question.. could cc::SurfaceLayer just show the background color as > solid color quads gutters from its AppendQuads? Does it have the info it would > need to do so? Would that be simpler or worse? I've thought about it, but currently the SurfaceLayer doesn't know the right size or the correct background color to use (because it doesn't get the frame data itself). So it's possible, but I don't know that it would actually make more sense. https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:289: current_frame_size_in_dip_.width(), On 2016/03/08 00:09:43, danakj wrote: > nit: put this into a |width| var too, it's nice to compute them both beside each > other? Done. https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:496: damage_rect_in_dip); On 2016/03/08 00:09:43, danakj wrote: > When if ever should damage include the gutters? Never, AFAIK. This is only supposed to use damage from the delegated frame, and in practice we use this only to detect whether a video is playing or not, so including the gutters doesn't make sense. https://codereview.chromium.org/1759323003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1759323003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1694: gfx::Size view_size(100, 100); On 2016/03/08 00:09:44, danakj wrote: > nit: large_size Done.
LGTM https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1759323003/diff/40001/content/browser/composi... content/browser/compositor/delegated_frame_host.cc:72: background_color_(SK_ColorWHITE), On 2016/03/08 01:45:29, jbauman wrote: > On 2016/03/08 00:09:43, danakj wrote: > > There's some work to be done to not show the wrong color during navigation: > > https://bugs.chromium.org/p/chromium/issues/detail?id=470669 > > > > And this adds yet another WHITE constant somewhere, as well as another place > > that decides on the right color to use, which I'm worried will cause some > weird > > stuff like black background white gutters sometimes or something. Can you > > alleviate my fears or find a way to get a shared value for this? > > Actually this initialization isn't really necessary (I just did it because I > wanted to be sure we weren't reading uninitialized memory in crazy cases - I > could remove it). Can you make it some more crazy noticeable color then? Red or Yellow or something. > We only create the gutters after we've received a frame from the renderer which > informs us of the metadata.root_background_color. That color was originally > intended to be used to create resize gutters, and we already use it for that > purpose on android. It's not guaranteed to always be correct, but in general it > seems pretty good.
jbauman@chromium.org changed reviewers: + piman@chromium.org
piman@: content/ OWNERS review please.
lgtm
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1759323003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759323003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759323003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jbauman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759323003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759323003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbauman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759323003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759323003/80001
Message was sent while issue was closed.
Description was changed from ========== Add resize gutter layers to DelegatedFrameHost If the current delegated frame size doesn't match the expected size of the renderer, use the body color from the renderer to create layers along the right and bottom sides to fill in the gap. This improves the appearance of resizing the new tab page and other pages with light backgrounds. BUG=505612 ========== to ========== Add resize gutter layers to DelegatedFrameHost If the current delegated frame size doesn't match the expected size of the renderer, use the body color from the renderer to create layers along the right and bottom sides to fill in the gap. This improves the appearance of resizing the new tab page and other pages with light backgrounds. BUG=505612 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add resize gutter layers to DelegatedFrameHost If the current delegated frame size doesn't match the expected size of the renderer, use the body color from the renderer to create layers along the right and bottom sides to fill in the gap. This improves the appearance of resizing the new tab page and other pages with light backgrounds. BUG=505612 ========== to ========== Add resize gutter layers to DelegatedFrameHost If the current delegated frame size doesn't match the expected size of the renderer, use the body color from the renderer to create layers along the right and bottom sides to fill in the gap. This improves the appearance of resizing the new tab page and other pages with light backgrounds. BUG=505612 Committed: https://crrev.com/3cb637dda82dc3df902f9341718a8a785368204b Cr-Commit-Position: refs/heads/master@{#380739} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3cb637dda82dc3df902f9341718a8a785368204b Cr-Commit-Position: refs/heads/master@{#380739} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
