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

Issue 1514723003: Fix bug where custom cursors would dissappear when screen was rotated (Closed)

Created:
5 years ago by kylechar
Modified:
5 years ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 Committed: https://crrev.com/fd9f15be2e290d54095d1b3c6b2285d89447e984 Cr-Commit-Position: refs/heads/master@{#366423}

Patch Set 1 #

Patch Set 2 : Change to actually convert SkBitmap #

Patch Set 3 : Improve comments. #

Patch Set 4 : Remove unnecessary CHECK() statement. #

Total comments: 2

Patch Set 5 : Fix typo in comment. #

Total comments: 2

Patch Set 6 : Fix variable name. #

Total comments: 1

Patch Set 7 : Change DCHECK to DCHECK_NE #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -0 lines) Patch
M ui/base/cursor/cursor_util.cc View 1 2 3 4 5 2 chunks +37 lines, -0 lines 0 comments Download
M ui/gfx/skbitmap_operations.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (11 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514723003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514723003/20001
5 years ago (2015-12-17 18:13:21 UTC) #2
commit-bot: I haz the power
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even ...
5 years ago (2015-12-17 18:13:23 UTC) #4
alexst (slow to review)
https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_util.cc File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_util.cc#newcode46 ui/base/cursor/cursor_util.cc:46: // SkBitmapOperations::Rotate() needs the the bitmap to have premultiplied ...
5 years ago (2015-12-17 20:56:28 UTC) #7
kylechar
Hi Mike, I was talking with sadrul@ and he mentioned you might know if there ...
5 years ago (2015-12-17 21:52:10 UTC) #9
reed1
lgtm just curious, why must we keep the cursor in unpremul form?
5 years ago (2015-12-17 22:13:44 UTC) #10
sadrul
https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_util.cc File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_util.cc#newcode22 ui/base/cursor/cursor_util.cc:22: bool ConvertSkBitmapAlphaType(SkBitmap* bitmap, SkAlphaType alphaType) { alpha_type (see https://google.github.io/styleguide/cppguide.html#Variable_Names)
5 years ago (2015-12-18 18:00:09 UTC) #12
kylechar
On 2015/12/17 22:13:44, reed1 wrote: > lgtm > > just curious, why must we keep ...
5 years ago (2015-12-18 18:10:13 UTC) #13
kylechar
https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_util.cc File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_util.cc#newcode22 ui/base/cursor/cursor_util.cc:22: bool ConvertSkBitmapAlphaType(SkBitmap* bitmap, SkAlphaType alphaType) { On 2015/12/18 18:00:09, ...
5 years ago (2015-12-18 18:10:31 UTC) #14
sadrul
lgtm Another option would be to do the necessary conversions in SkBitmapOperations::Rotate() instead. But we ...
5 years ago (2015-12-21 16:32:12 UTC) #15
kylechar
On 2015/12/21 at 16:32:12, sadrul wrote: > lgtm > > Another option would be to ...
5 years ago (2015-12-21 17:23:18 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514723003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514723003/140001
5 years ago (2015-12-21 17:26:23 UTC) #20
commit-bot: I haz the power
Committed patchset #7 (id:140001)
5 years ago (2015-12-21 18:43:08 UTC) #22
commit-bot: I haz the power
5 years ago (2015-12-21 18:44:04 UTC) #24
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/fd9f15be2e290d54095d1b3c6b2285d89447e984
Cr-Commit-Position: refs/heads/master@{#366423}

Powered by Google App Engine
This is Rietveld 408576698