Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(15)

Issue 2090003003: Remove NativeThemeWin::PaintIndirect (Closed)

Created:
4 years, 6 months ago by tomhudson
Modified:
4 years, 5 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, enne (OOO), reveman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove NativeThemeWin::PaintIndirect We don't seem to actually use this complicated codepath in practice. In particular: 1. // This block will only get hit with --enable-accelerated-drawing flag. That flag is obsolete, and its replacement also seems to no longer exist. 2. // Scrollbar components on Windows Classic theme (on all Windows versions) // have particularly problematic alpha values, so always draw them // indirectly. Turning on the Windows Classic theme on 64b Windows 7, I can't cause the block to be hit today. 3. // In addition, scrollbar thumbs and grippers for the Windows XP // theme (available only on Windows XP) also need their alpha values // fixed. We're no longer supporting XP and have been cleared to remove relevant code. Once it's gone, so are several copies of functions that paint the same UI element in different ways depending on whether they're drawing to HDC or SkCanvas: Gutter, MenuSeparator, MenuBackground, and MenuItemBackground. R=pkasting@chromium.org BUG=543755, 579196, 622692 Committed: https://crrev.com/83a3c26417896b207efe22b116a03a9a162c0ca5 Cr-Commit-Position: refs/heads/master@{#404155}

Patch Set 1 #

Total comments: 2

Patch Set 2 : review #

Total comments: 2

Patch Set 3 : switch to default: #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -294 lines) Patch
M ui/native_theme/native_theme_win.h View 1 4 chunks +0 lines, -28 lines 0 comments Download
M ui/native_theme/native_theme_win.cc View 1 2 7 chunks +66 lines, -266 lines 0 comments Download

Messages

Total messages: 27 (13 generated)
tomhudson
If we can remove this code it will get rid of most of the functional ...
4 years, 6 months ago (2016-06-23 19:26:04 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2090003003/1
4 years, 6 months ago (2016-06-23 20:10:12 UTC) #7
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-23 20:50:59 UTC) #9
tomhudson
+estade@ since Peter's set to OOO
4 years, 6 months ago (2016-06-23 21:54:23 UTC) #11
Evan Stade
I'm technically an owner of this code but I can't provide a meaningful review of ...
4 years, 6 months ago (2016-06-24 16:07:20 UTC) #12
Peter Kasting
https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_theme_win.cc File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_theme_win.cc#newcode24 ui/native_theme/native_theme_win.cc:24: #include "third_party/skia/include/core/SkColorPriv.h" Maybe can remove this #include now? https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_theme_win.cc#newcode258 ...
4 years, 5 months ago (2016-06-29 23:20:01 UTC) #13
tomhudson
On 2016/06/29 23:20:01, Peter Kasting wrote: > https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_theme_win.cc > File ui/native_theme/native_theme_win.cc (right): > > https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_theme_win.cc#newcode24 ...
4 years, 5 months ago (2016-07-06 13:13:16 UTC) #16
Peter Kasting
LGTM https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_theme_win.cc File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_theme_win.cc#newcode327 ui/native_theme/native_theme_win.cc:327: case kScrollbarCorner: Nit: Put these in enum order ...
4 years, 5 months ago (2016-07-06 21:04:34 UTC) #17
tomhudson
https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_theme_win.cc File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_theme_win.cc#newcode327 ui/native_theme/native_theme_win.cc:327: case kScrollbarCorner: On 2016/07/06 21:04:33, Peter Kasting wrote: > ...
4 years, 5 months ago (2016-07-07 12:51:51 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2090003003/40001
4 years, 5 months ago (2016-07-07 12:52:04 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 5 months ago (2016-07-07 14:15:31 UTC) #23
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-07 14:15:40 UTC) #24
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/83a3c26417896b207efe22b116a03a9a162c0ca5 Cr-Commit-Position: refs/heads/master@{#404155}
4 years, 5 months ago (2016-07-07 14:16:57 UTC) #26
tomhudson
4 years, 4 months ago (2016-07-26 00:28:54 UTC) #27
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2181153002/ by tomhudson@google.com.

The reason for reverting is: Directly causes crbug.com/628574, which is
ship-blocking; we'll have to find another way to clean up the mess..

Powered by Google App Engine
This is Rietveld 408576698