Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804983003:
[blink] Use early returns in baseBackgroundColor(Override) setters. (Closed)
|
Created:
3 years, 8 months ago by Eric Seckler Modified:
3 years, 8 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-frames_chromium.org, chromium-reviews, dcheng, kinuko+watch, mlamouri+watch-blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[blink] Use early returns in baseBackgroundColor(Override) setters.
The patch adds early outs for set/clearBaseBackgroundColorOverride
and FrameView::setBaseBackgroundColor to avoid unnecessary updates.
It also adds a missing lifecycle update to the setters in
WebFrameWidgetImpl, which may otherwise lead to crashes in
isAllowedToQueryCompositingState().
BUG=689349
Review-Url: https://codereview.chromium.org/2804983003
Cr-Commit-Position: refs/heads/master@{#463127}
Committed: https://chromium.googlesource.com/chromium/src/+/d374c7111b60395ad0a715f5e1c0ddbb41c802af
Patch Set 1 #
Total comments: 5
Patch Set 2 : Revert compositing update fix, keep other changes. #
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by eseckler@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...
Description was changed from ========== [blink] Request compositing update in FrameView::setBaseBackgroundColor. In situations where there are multiple layers in FrameView, setBaseBackgroundColor was previously only requesting a display of the main layer, causing it to draw over any other layers. Calling PaintLayerCompositor::setNeedsCompositingUpdate when the base background color changes fixes this issue. The patch also adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. BUG=689349, 708445 ========== to ========== [blink] Request compositing update in FrameView::setBaseBackgroundColor. In situations where there are multiple layers in FrameView, setBaseBackgroundColor was previously only requesting a display of the main layer, causing it to potentially draw over other layers. Calling PaintLayerCompositor::setNeedsCompositingUpdate when the base background color changes fixes this issue. The patch also adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. BUG=689349, 708445 ==========
eseckler@chromium.org changed reviewers: + chrishtr@chromium.org
Chris, PTAL. I believe we don't need any of the previous compositedLayerMapping updates if we use setNeedsCompositingUpdate? Thank you!
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/2804983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2353: compositedLayerMapping->mainGraphicsLayer()->setNeedsDisplay(); You still need setNeedsDisplay in order to force invalidation of the graphics layer backing. setNeedsCompositingUpdate just forces the compositing step to recompute things like the graphics layer tree and opacity settings. https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:163: static void setNeedsCompositingUpdate(LayoutViewItem layoutViewItem, I don't see much point in making this a helper method - inline instead?
Also - this needs a test.
On 2017/04/06 17:34:04, chrishtr wrote: > Also - this needs a test. Do you have a pointer as to how I'd go about writing a test for this? :) unit test that checks that the needs update bit is set? or should I try to replicate the canvas problem? https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2353: compositedLayerMapping->mainGraphicsLayer()->setNeedsDisplay(); On 2017/04/06 17:33:46, chrishtr wrote: > You still need setNeedsDisplay in order to force invalidation of the graphics > layer backing. setNeedsCompositingUpdate just forces the compositing step > to recompute things like the graphics layer tree and opacity settings. Thanks, will do! https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:163: static void setNeedsCompositingUpdate(LayoutViewItem layoutViewItem, On 2017/04/06 17:33:46, chrishtr wrote: > I don't see much point in making this a helper method - inline instead? It was one before - I just moved it. Since it's used in a couple of places, keep it?
On 2017/04/06 at 17:40:29, eseckler wrote: > On 2017/04/06 17:34:04, chrishtr wrote: > > Also - this needs a test. > Do you have a pointer as to how I'd go about writing a test for this? :) unit test that checks that the needs update bit is set? or should I try to replicate the canvas problem? A unit test should be able to cover it, which simulates the sequence of actions leading to the behavior in the bug. > > https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (left): > > https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:2353: compositedLayerMapping->mainGraphicsLayer()->setNeedsDisplay(); > On 2017/04/06 17:33:46, chrishtr wrote: > > You still need setNeedsDisplay in order to force invalidation of the graphics > > layer backing. setNeedsCompositingUpdate just forces the compositing step > > to recompute things like the graphics layer tree and opacity settings. > > Thanks, will do! > > https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:163: static void setNeedsCompositingUpdate(LayoutViewItem layoutViewItem, > On 2017/04/06 17:33:46, chrishtr wrote: > > I don't see much point in making this a helper method - inline instead? > > It was one before - I just moved it. Since it's used in a couple of places, keep it?
https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2804983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:163: static void setNeedsCompositingUpdate(LayoutViewItem layoutViewItem, On 2017/04/06 at 17:40:29, Eric Seckler wrote: > On 2017/04/06 17:33:46, chrishtr wrote: > > I don't see much point in making this a helper method - inline instead? > > It was one before - I just moved it. Since it's used in a couple of places, keep it? Oh, didn't see that. Ignore my comment.
Description was changed from ========== [blink] Request compositing update in FrameView::setBaseBackgroundColor. In situations where there are multiple layers in FrameView, setBaseBackgroundColor was previously only requesting a display of the main layer, causing it to potentially draw over other layers. Calling PaintLayerCompositor::setNeedsCompositingUpdate when the base background color changes fixes this issue. The patch also adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. BUG=689349, 708445 ========== to ========== [blink] Use early returns in baseBackgroundColor(Override) setters. The patch also adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. It also adds a missing lifecycle update to the setters in WebFrameWidgetImpl, which may otherwise lead to crashes in isAllowedToQueryCompositingState(). BUG=689349 ==========
The CQ bit was checked by eseckler@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...
Removed the call to setNeedsCompositingUpdate again and repurposed the patch -- PTAL :)
Description was changed from ========== [blink] Use early returns in baseBackgroundColor(Override) setters. The patch also adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. It also adds a missing lifecycle update to the setters in WebFrameWidgetImpl, which may otherwise lead to crashes in isAllowedToQueryCompositingState(). BUG=689349 ========== to ========== [blink] Use early returns in baseBackgroundColor(Override) setters. The patch adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. It also adds a missing lifecycle update to the setters in WebFrameWidgetImpl, which may otherwise lead to crashes in isAllowedToQueryCompositingState(). BUG=689349 ==========
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 eseckler@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": 20001, "attempt_start_ts": 1491634740474450,
"parent_rev": "9d8fbbd04c54aeb0efab702bb1912ed968dd7ca4", "commit_rev":
"d374c7111b60395ad0a715f5e1c0ddbb41c802af"}
Message was sent while issue was closed.
Description was changed from ========== [blink] Use early returns in baseBackgroundColor(Override) setters. The patch adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. It also adds a missing lifecycle update to the setters in WebFrameWidgetImpl, which may otherwise lead to crashes in isAllowedToQueryCompositingState(). BUG=689349 ========== to ========== [blink] Use early returns in baseBackgroundColor(Override) setters. The patch adds early outs for set/clearBaseBackgroundColorOverride and FrameView::setBaseBackgroundColor to avoid unnecessary updates. It also adds a missing lifecycle update to the setters in WebFrameWidgetImpl, which may otherwise lead to crashes in isAllowedToQueryCompositingState(). BUG=689349 Review-Url: https://codereview.chromium.org/2804983003 Cr-Commit-Position: refs/heads/master@{#463127} Committed: https://chromium.googlesource.com/chromium/src/+/d374c7111b60395ad0a715f5e1c0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d374c7111b60395ad0a715f5e1c0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
