Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808123004:
[devtools] Remove device emulation background color override. (Closed)
|
Created:
3 years, 8 months ago by Eric Seckler Modified:
3 years, 8 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org, Sami Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[devtools] Remove device emulation background color override.
If used in combination with a transparent setDefaultBackgroundColor,
the background color override applied by setDeviceMetricsOverride is
harmful. The background override does not seem to be beneficial
anymore, as the frontend already doesn't use it.
BUG=689349
Review-Url: https://codereview.chromium.org/2808123004
Cr-Commit-Position: refs/heads/master@{#464335}
Committed: https://chromium.googlesource.com/chromium/src/+/9f00423812b73080001e1742d6ad356c68c23b62
Patch Set 1 : . #
Total comments: 2
Patch Set 2 : remove background color override #Patch Set 3 : fix android tests #
Total comments: 4
Messages
Total messages: 32 (23 generated)
Patchset #1 (id:1) has been deleted
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...
eseckler@chromium.org changed reviewers: + dgozman@chromium.org
Dmitry, PTAL. Let me know if you'd like to solve this differently :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2808123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/2808123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DevToolsEmulator.cpp:230: web_view_impl_->SetBackgroundColorOverride(Color::kDarkGray); Let's just not override at all. DevTools currently does not benefit from this, and we have a different method for RDP users.
Description was changed from ========== [devtools] Make gutter color override in device emulation configurable. If used in combination with a transparent setDefaultBackgroundColor, the background color override applied by setDeviceMetricsOverride can be harmful. This patch adds an option to turn the color override of. BUG=689349 ========== to ========== [devtools] Remove device emulation background color override. If used in combination with a transparent setDefaultBackgroundColor, the background color override applied by setDeviceMetricsOverride is harmful. The background override does not seem to be beneficial anymore, as the frontend already doesn't use it. BUG=689349 ==========
https://codereview.chromium.org/2808123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/2808123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DevToolsEmulator.cpp:230: web_view_impl_->SetBackgroundColorOverride(Color::kDarkGray); On 2017/04/11 17:29:54, dgozman wrote: > Let's just not override at all. DevTools currently does not benefit from this, > and we have a different method for RDP users. Perfect! Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:60001) has been deleted
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/2808123004/diff/80001/content/renderer/devtoo... File content/renderer/devtools/render_widget_screen_metrics_emulator.cc (left): https://codereview.chromium.org/2808123004/diff/80001/content/renderer/devtoo... content/renderer/devtools/render_widget_screen_metrics_emulator.cc:148: modified_resize_params.physical_backing_size = @Dmirty: This was causing a test failure b/c of an off-by-one rounding error (see android bot on patch set 2). I don't think that this statement is actually necessary, since it seems to (inaccurately) recompute the same value that would have been set in original_resize_params_ already. But maybe I'm missing something - WDYT?
lgtm, but let's cleanup in WebViewImpl as well https://codereview.chromium.org/2808123004/diff/80001/content/renderer/devtoo... File content/renderer/devtools/render_widget_screen_metrics_emulator.cc (left): https://codereview.chromium.org/2808123004/diff/80001/content/renderer/devtoo... content/renderer/devtools/render_widget_screen_metrics_emulator.cc:148: modified_resize_params.physical_backing_size = On 2017/04/12 16:18:46, Eric Seckler wrote: > @Dmirty: This was causing a test failure b/c of an off-by-one rounding error > (see android bot on patch set 2). I don't think that this statement is actually > necessary, since it seems to (inaccurately) recompute the same value that would > have been set in original_resize_params_ already. But maybe I'm missing > something - WDYT? You are right. I guess this has evolved to be useless now :-) https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left): https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DevToolsEmulator.cpp:230: web_view_impl_->SetBackgroundColorOverride(Color::kDarkGray); Remove these methods from WebViewImpl as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left): https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DevToolsEmulator.cpp:230: web_view_impl_->SetBackgroundColorOverride(Color::kDarkGray); On 2017/04/12 18:21:23, dgozman wrote: > Remove these methods from WebViewImpl as well? After my recent refactorings of transparency overrides, these methods are now actually used for a non-devtools purpose, too: https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?l=2240
On 2017/04/12 19:31:31, Eric Seckler wrote: > https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left): > > https://codereview.chromium.org/2808123004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/DevToolsEmulator.cpp:230: > web_view_impl_->SetBackgroundColorOverride(Color::kDarkGray); > On 2017/04/12 18:21:23, dgozman wrote: > > Remove these methods from WebViewImpl as well? > > After my recent refactorings of transparency overrides, these methods are now > actually used for a non-devtools purpose, too: > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?l=2240 Oh, nice! I didn't know :)
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": 80001, "attempt_start_ts": 1492069124108940,
"parent_rev": "df702e868c39760f3d62bdc0dd75b1987a1c4799", "commit_rev":
"9f00423812b73080001e1742d6ad356c68c23b62"}
Message was sent while issue was closed.
Description was changed from ========== [devtools] Remove device emulation background color override. If used in combination with a transparent setDefaultBackgroundColor, the background color override applied by setDeviceMetricsOverride is harmful. The background override does not seem to be beneficial anymore, as the frontend already doesn't use it. BUG=689349 ========== to ========== [devtools] Remove device emulation background color override. If used in combination with a transparent setDefaultBackgroundColor, the background color override applied by setDeviceMetricsOverride is harmful. The background override does not seem to be beneficial anymore, as the frontend already doesn't use it. BUG=689349 Review-Url: https://codereview.chromium.org/2808123004 Cr-Commit-Position: refs/heads/master@{#464335} Committed: https://chromium.googlesource.com/chromium/src/+/9f00423812b73080001e1742d6ad... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9f00423812b73080001e1742d6ad... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
