|
|
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. |
DescriptionRemove 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: #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Remove NativeThemeWin::PaintIndirect We don't seem to actually use this complicated codepath in practice. BUG=543755 ========== to ========== 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. Passing that flag and launching Chrome, I can't cause the block to be hit today. 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. BUG=543755,579196 ==========
Description was changed from ========== 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. Passing that flag and launching Chrome, I can't cause the block to be hit today. 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. BUG=543755,579196 ========== to ========== 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. Passing that flag and launching Chrome, I can't cause the block to be hit today. 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. BUG=543755,579196,622692 ==========
Description was changed from ========== 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. Passing that flag and launching Chrome, I can't cause the block to be hit today. 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. BUG=543755,579196,622692 ========== to ========== 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. BUG=543755,579196,622692 ==========
tomhudson@google.com changed reviewers: + pkasting@chromium.org
If we can remove this code it will get rid of most of the functional complexity of https://codereview.chromium.org/1492353002/, leaving just cleanup around CreateBitmapHeader() and related functions.
The CQ bit was checked by tomhudson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2090003003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tomhudson@google.com changed reviewers: + estade@chromium.org
+estade@ since Peter's set to OOO
I'm technically an owner of this code but I can't provide a meaningful review of this windows-specific codepath so you may need to wait for Peter's return (I think that will be next week?)
https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_them... 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_them... ui/native_theme/native_theme_win.cc:258: PaintDirect(canvas, part, state, rect, extra); This is now the only caller of this function. I would combine the two into one function. This should allow eliminating all the handling code in that function for the four cases covered in the switch above, I believe.
Description was changed from ========== 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. BUG=543755,579196,622692 ========== to ========== 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 ==========
tomhudson@google.com changed reviewers: - estade@chromium.org
On 2016/06/29 23:20:01, Peter Kasting wrote: > https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_them... > File ui/native_theme/native_theme_win.cc (right): > > https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_win.cc:24: #include > "third_party/skia/include/core/SkColorPriv.h" > Maybe can remove this #include now? Yes, and SkRefCnt.h - good catch. > https://codereview.chromium.org/2090003003/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_win.cc:258: PaintDirect(canvas, part, state, rect, > extra); > This is now the only caller of this function. I would combine the two into one > function. > > This should allow eliminating all the handling code in that function for the > four cases covered in the switch above, I believe. PTAL; I kept two separate switches in this version of the CL and moved some additional code up there because if we can draw directly to the SkCanvas we're going to be a bit more performant than if we have to grab the HDC, and this keeps the two kinds of draws separate. Following up *did* allow me to eliminate several HDC-targeting versions of paint functions that are now always going through their Canvas-targeting equivalents. I likely wouldn't have done this without more justification if we didn't already have two switches, so I'm willing to recombine the two if you think it's premature optimization that doesn't add anything to the clarity, but I like the separation of draw target types even if there wasn't a potential performance difference.
LGTM https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:327: case kScrollbarCorner: Nit: Put these in enum order within this group. Personally I would just use "default:" instead of all these -- I find it more annoying than helpful that this particular switch verifies that every enum value gets checked, since any parts we add in the future are so likely to want to be no-ops here -- but up to you.
https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2090003003/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:327: case kScrollbarCorner: On 2016/07/06 21:04:33, Peter Kasting wrote: > Nit: Put these in enum order within this group. > > Personally I would just use "default:" instead of all these -- I find it more > annoying than helpful that this particular switch verifies that every enum value > gets checked, since any parts we add in the future are so likely to want to be > no-ops here -- but up to you. Acknowledged. Changed to default:
The CQ bit was checked by tomhudson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2090003003/#ps40001 (title: "switch to default:")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/83a3c26417896b207efe22b116a03a9a162c0ca5 Cr-Commit-Position: refs/heads/master@{#404155}
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.. |