Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2643723008:
[devtools] Add a command to emulate the default background color. (Closed)
|
Created:
3 years, 11 months ago by Eric Seckler Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kinuko+watch, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[devtools] Add a command to emulate the default background color.
BUG=683073
Review-Url: https://codereview.chromium.org/2643723008
Cr-Commit-Position: refs/heads/master@{#448557}
Committed: https://chromium.googlesource.com/chromium/src/+/9e1762d63c728455a9a45f3541097f73118db2a7
Patch Set 1 #
Total comments: 8
Patch Set 2 : Try using setBackgroundColorOverride #
Total comments: 2
Patch Set 3 : back to setBaseBackgroundColor. also adds state restore/reset. #
Total comments: 2
Patch Set 4 : store override in WebViewImpl. #Patch Set 5 : Apply compositedLayerMapping updates during updateLifecyclePhases to fix DCHECK errors. #
Total comments: 2
Patch Set 6 : Force lifecycle update to CompositingCleanPlusScrolling. #Messages
Total messages: 45 (26 generated)
eseckler@chromium.org changed reviewers: + dgozman@chromium.org
Dmitry, I'm adding this to Emulation, since that seems most sensible. WDYT? Thanks!
https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html:7: background-color: #123456; Looks like the page here specifies the background, which is the opposite to test description. Also, where is an image expectation? https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:176: std::unique_ptr<protocol::DOM::RGBA> color) { I'd like this to be optional, so that passing nothing resets to default one. https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:179: m_webLocalFrameImpl->frameWidget()->setBaseBackgroundColor( We use WebViewImpl::setBackgroundColorOverride in DevToolsEmulator. Should we merge two?
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...
Thanks, Dmitry. PTAL :) https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html:7: background-color: #123456; On 2017/01/20 17:57:07, dgozman wrote: > Looks like the page here specifies the background, which is the opposite to test > description. > Also, where is an image expectation? This is the expectation file, which specifies the background color to generate the reference image :) The test file (without specified background color) is above. https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:176: std::unique_ptr<protocol::DOM::RGBA> color) { On 2017/01/20 17:57:07, dgozman wrote: > I'd like this to be optional, so that passing nothing resets to default one. Done. Also renamed the command to overrideDefaultBackgroundColor https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:179: m_webLocalFrameImpl->frameWidget()->setBaseBackgroundColor( On 2017/01/20 17:57:07, dgozman wrote: > We use WebViewImpl::setBackgroundColorOverride in DevToolsEmulator. Should we > merge two? Sounds good. I'm trying to use this method now, but it seems that it doesn't have the effect I'm looking for - or I'm doing something wrong. The test, at least, doesn't pass anymore, because the page stays white. Can you spot a mistake? :)
Thanks, Dmitry. PTAL :) https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/emulation/default-background-color-expected.html:7: background-color: #123456; On 2017/01/20 17:57:07, dgozman wrote: > Looks like the page here specifies the background, which is the opposite to test > description. > Also, where is an image expectation? This is the expectation file, which specifies the background color to generate the reference image :) The test file (without specified background color) is above. https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:176: std::unique_ptr<protocol::DOM::RGBA> color) { On 2017/01/20 17:57:07, dgozman wrote: > I'd like this to be optional, so that passing nothing resets to default one. Done. Also renamed the command to overrideDefaultBackgroundColor https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:179: m_webLocalFrameImpl->frameWidget()->setBaseBackgroundColor( On 2017/01/20 17:57:07, dgozman wrote: > We use WebViewImpl::setBackgroundColorOverride in DevToolsEmulator. Should we > merge two? Sounds good. I'm trying to use this method now, but it seems that it doesn't have the effect I'm looking for - or I'm doing something wrong. The test, at least, doesn't pass anymore, because the page stays white. Can you spot a mistake? :)
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_...)
https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:179: m_webLocalFrameImpl->frameWidget()->setBaseBackgroundColor( On 2017/01/23 18:06:24, Eric Seckler wrote: > On 2017/01/20 17:57:07, dgozman wrote: > > We use WebViewImpl::setBackgroundColorOverride in DevToolsEmulator. Should we > > merge two? > > Sounds good. I'm trying to use this method now, but it seems that it doesn't > have the effect I'm looking for - or I'm doing something wrong. The test, at > least, doesn't pass anymore, because the page stays white. > > Can you spot a mistake? :) Hmm... Looks like they are different. The one we use in device emulation is for area around the page, while you want to override the page background itself. Sorry for confusion. https://codereview.chromium.org/2643723008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2643723008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:840: "name": "overrideDefaultBackgroundColor", setDefaultBackgroundColorOverride
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...
PTAL :) https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:179: m_webLocalFrameImpl->frameWidget()->setBaseBackgroundColor( On 2017/01/23 20:27:04, dgozman wrote: > On 2017/01/23 18:06:24, Eric Seckler wrote: > > On 2017/01/20 17:57:07, dgozman wrote: > > > We use WebViewImpl::setBackgroundColorOverride in DevToolsEmulator. Should > we > > > merge two? > > > > Sounds good. I'm trying to use this method now, but it seems that it doesn't > > have the effect I'm looking for - or I'm doing something wrong. The test, at > > least, doesn't pass anymore, because the page stays white. > > > > Can you spot a mistake? :) > > Hmm... Looks like they are different. The one we use in device emulation is for > area around the page, while you want to override the page background itself. > Sorry for confusion. Ah, okay, that makes sense then. Back to setBaseBackgroundColor it is :) FYI, I'm now also storing this override in the agent's state to support restoring and resetting it. https://codereview.chromium.org/2643723008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2643723008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:840: "name": "overrideDefaultBackgroundColor", On 2017/01/23 20:27:04, dgozman wrote: > setDefaultBackgroundColorOverride Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2643723008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:196: m_state->get(EmulationAgentState::defaultBackgroundColorOriginalRGBA); You don't have to store original in the state. Instead, you should have an override in WebViewImpl (similar to another one we have) and just clear it from here. That way, WebViewImpl would know about the default one and the override one and prefer one to another.
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...
https://codereview.chromium.org/2643723008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2643723008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:196: m_state->get(EmulationAgentState::defaultBackgroundColorOriginalRGBA); On 2017/01/24 20:12:49, dgozman wrote: > You don't have to store original in the state. Instead, you should have an > override in WebViewImpl (similar to another one we have) and just clear it from > here. That way, WebViewImpl would know about the default one and the override > one and prefer one to another. Thanks! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 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...
Turns out we may be triggering a DCHECK error, because setting the FrameView::setBaseBackgroundColor() accesses the compositedLayerMapping, which should only be done while allowed to query compositing state. I'm circumventing this by moving said compositedLayerMapping-related things to happen during updateLifecyclePhases instead. Does that make sense?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dgozman@chromium.org changed reviewers: + chrishtr@chromium.org
+chrishtr for compositedLayerMapping stuff in FrameView. The rest lgtm. https://codereview.chromium.org/2643723008/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2643723008/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:3596: WebColor color = alphaChannel(m_baseBackgroundColorOverride) Use baseBackgroundColor().
Some questions: Why these changes to FrameView? Does it fail to invalidate and update certain things otherwise? Also, please don't add a new dirty bit. Instead, use setNeedsDisplay on the appropriate GraphicsLayer and then call compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree). It appears this will force recomputation of opaqueness for you.
On 2017/01/26 00:50:07, chrishtr wrote: > Some questions: > > Why these changes to FrameView? Does it fail to invalidate and update certain > things otherwise? No, but a DCHECK in layoutViewItem().layer()->compositedLayerMapping() may be triggered depending on where we call FrameView::setBaseBackgroundColor() from. Said DCHECK verifies that PaintLayer::isAllowedToQueryCompositingState() is true. Given that we are calling FrameView::setBaseBackgroundColor() from the DevTools agent, this might not be the case, as evidenced by the bot failures on patchset #4. The likely reason this wasn't a problem before is that setBaseBackgroundColor() is currently only called during construction of the FrameView. > Also, please don't add a new dirty bit. Instead, use setNeedsDisplay on the > appropriate > GraphicsLayer and then call > compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree). > It appears this will force recomputation of opaqueness for you. Would this GraphicsLayer be the mainGraphicsLayer() of the compositedLayerMapping? If so, that doesn't really help avoiding accessing compositedLayerMapping(). Btw, FrameView::setTransparent() seems to get around the DCHECK by simply disabling it using DisableCompositingQueryAsserts. Would that make sense for setBaseBackgroundColor(), too?
On 2017/01/26 09:59:06, Eric Seckler wrote: > On 2017/01/26 00:50:07, chrishtr wrote: > > Some questions: > > > > Why these changes to FrameView? Does it fail to invalidate and update certain > > things otherwise? > No, but a DCHECK in layoutViewItem().layer()->compositedLayerMapping() may be > triggered depending on where we call FrameView::setBaseBackgroundColor() from. > Said DCHECK verifies that PaintLayer::isAllowedToQueryCompositingState() is > true. > > Given that we are calling FrameView::setBaseBackgroundColor() from the DevTools > agent, this might not be the case, as evidenced by the bot failures on patchset > #4. > The likely reason this wasn't a problem before is that setBaseBackgroundColor() > is currently only called during construction of the FrameView. > > > Also, please don't add a new dirty bit. Instead, use setNeedsDisplay on the > > appropriate > > GraphicsLayer and then call > > compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree). > > It appears this will force recomputation of opaqueness for you. > Would this GraphicsLayer be the mainGraphicsLayer() of the > compositedLayerMapping? If so, that doesn't really help avoiding accessing > compositedLayerMapping(). > > Btw, FrameView::setTransparent() seems to get around the DCHECK by simply > disabling it using DisableCompositingQueryAsserts. Would that make sense for > setBaseBackgroundColor(), too? Chris, Can I ask you for advice on this? Thanks!
On 2017/02/06 at 22:26:19, eseckler wrote: > On 2017/01/26 09:59:06, Eric Seckler wrote: > > On 2017/01/26 00:50:07, chrishtr wrote: > > > Some questions: > > > > > > Why these changes to FrameView? Does it fail to invalidate and update certain > > > things otherwise? > > No, but a DCHECK in layoutViewItem().layer()->compositedLayerMapping() may be > > triggered depending on where we call FrameView::setBaseBackgroundColor() from. > > Said DCHECK verifies that PaintLayer::isAllowedToQueryCompositingState() is > > true. I see. Why not just force-update the lifecycle to CompositingCleanPlusScrolling before calling setBaseBackgroundColor? > > > > Given that we are calling FrameView::setBaseBackgroundColor() from the DevTools > > agent, this might not be the case, as evidenced by the bot failures on patchset > > #4. > > The likely reason this wasn't a problem before is that setBaseBackgroundColor() > > is currently only called during construction of the FrameView. > > > > > Also, please don't add a new dirty bit. Instead, use setNeedsDisplay on the > > > appropriate > > > GraphicsLayer and then call > > > compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree). > > > It appears this will force recomputation of opaqueness for you. > > Would this GraphicsLayer be the mainGraphicsLayer() of the > > compositedLayerMapping? If so, that doesn't really help avoiding accessing > > compositedLayerMapping(). I think you're referring to the lifecycle assert again here right? > > > > Btw, FrameView::setTransparent() seems to get around the DCHECK by simply > > disabling it using DisableCompositingQueryAsserts. Would that make sense for > > setBaseBackgroundColor(), too? That one is a bug, perhaps that can be cleaned up with the same technique (not forcing you to, but it would be nice). > > Chris, Can I ask you for advice on this? Thanks! Sorry for the slow reply. I was very sick most of last week.
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...
PTAL :) On 2017/02/06 22:59:14, chrishtr wrote: > I see. Why not just force-update the lifecycle to CompositingCleanPlusScrolling > before > calling setBaseBackgroundColor? Thanks, that sounds like a reasonable solution :) > > > Btw, FrameView::setTransparent() seems to get around the DCHECK by simply > > > disabling it using DisableCompositingQueryAsserts. Would that make sense for > > > setBaseBackgroundColor(), too? > > That one is a bug, perhaps that can be cleaned up with the same technique (not > forcing you to, but it would be nice). Not sure when/how the DCHECK would be triggered there, i.e. where we'd add the call to update the lifecycle. > Sorry for the slow reply. I was very sick most of last week. No worries, hope you feel better! https://codereview.chromium.org/2643723008/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2643723008/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:3596: WebColor color = alphaChannel(m_baseBackgroundColorOverride) On 2017/01/26 00:04:17, dgozman (OOO till Feb 13) wrote: > Use baseBackgroundColor(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2643723008/#ps100001 (title: "Force lifecycle update to CompositingCleanPlusScrolling.")
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": 100001, "attempt_start_ts": 1486445500640620,
"parent_rev": "1dc1289009fc9dab98f5f840ff342ecaa1238c98", "commit_rev":
"9e1762d63c728455a9a45f3541097f73118db2a7"}
Message was sent while issue was closed.
Description was changed from ========== [devtools] Add a command to emulate the default background color. BUG=683073 ========== to ========== [devtools] Add a command to emulate the default background color. BUG=683073 Review-Url: https://codereview.chromium.org/2643723008 Cr-Commit-Position: refs/heads/master@{#448557} Committed: https://chromium.googlesource.com/chromium/src/+/9e1762d63c728455a9a45f354109... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9e1762d63c728455a9a45f354109...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 448557 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
