Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268563003:
Replaced CursorLoader in PlatformDisplay with a ImageCursor (Closed)
DescriptionReplaced CursorLoader in PlatformDisplay with a ImageCursor
ImageCursor->SetDisplay() is called in Init() so that all cursors are loaded at
launch. This fixes the bug where the cursor image is positioned off to the left
in Chrome mash.
BUG=615861
Committed: https://crrev.com/41725e3dd88eadb07f8a1297c0f8f6ecb9b30c79
Cr-Commit-Position: refs/heads/master@{#439154}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add display::Display PlatformDisplayDelegate::GetDisplay() #
Total comments: 4
Patch Set 3 : Merged namespace blocks and replaced static_cast with GetDisplay() #Patch Set 4 : Moved ImageCursors::SetDisplay() to the end of DefaultPlatformDisplay::Init() #Patch Set 5 : rebase #Patch Set 6 : Rebase #
Messages
Total messages: 60 (32 generated)
staraz@chromium.org changed reviewers: + sky@chromium.org
Please review my changes.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... File services/ui/ws/platform_display.cc (right): https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.cc:85: GetDeviceScaleFactor()); Instead of static casting, introduce display::Display PlatformDisplayDelegate::GetDisplay() https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.cc:143: // cursor_loader_->SetPlatformCursor(&cursor); Remove. https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... File services/ui/ws/platform_display.h (right): https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.h:168: // std::unique_ptr<ui::CursorLoader> cursor_loader_; Remove
https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... File services/ui/ws/platform_display.cc (right): https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.cc:85: GetDeviceScaleFactor()); On 2016/08/22 14:39:40, sadrul wrote: > Instead of static casting, introduce display::Display > PlatformDisplayDelegate::GetDisplay() Done. https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.cc:143: // cursor_loader_->SetPlatformCursor(&cursor); On 2016/08/22 14:39:40, sadrul wrote: > Remove. Done. https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... File services/ui/ws/platform_display.h (right): https://codereview.chromium.org/2268563003/diff/1/services/ui/ws/platform_dis... services/ui/ws/platform_display.h:168: // std::unique_ptr<ui::CursorLoader> cursor_loader_; On 2016/08/22 14:39:40, sadrul wrote: > Remove Done.
https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display.cc (right): https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display.cc:84: static_cast<ui::ws::Display*>(delegate_)->ToDisplay(), Use the newly added delegate function. https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display.h (right): https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display.h:40: } // namespace ui whoa. While you are here, mind merging this down to the block below? Remove CursorLoader, since that's no longer used.
https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display.cc (right): https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display.cc:84: static_cast<ui::ws::Display*>(delegate_)->ToDisplay(), On 2016/08/22 15:18:14, sadrul wrote: > Use the newly added delegate function. Done. https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display.h (right): https://codereview.chromium.org/2268563003/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display.h:40: } // namespace ui On 2016/08/22 15:18:14, sadrul wrote: > whoa. While you are here, mind merging this down to the block below? > > Remove CursorLoader, since that's no longer used. Done.
lgtm
LGTM
The CQ bit was checked by staraz@chromium.org
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
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 staraz@chromium.org
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
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_...)
The CQ bit was checked by staraz@chromium.org
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
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_...)
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2268563003/#ps60001 (title: "Moved ImageCursors::SetDisplay() to the end of DefaultPlatformDisplay::Init()")
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
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_...)
The CQ bit was checked by staraz@chromium.org
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
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_...)
The CQ bit was checked by sadrul@chromium.org
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
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_...)
The CQ bit was checked by sadrul@chromium.org
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
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_...)
The CQ bit was checked by staraz@chromium.org
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
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_...) linux_chromium_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 staraz@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_chromium_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 staraz@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: This issue passed the CQ dry run.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2268563003/#ps100001 (title: "Rebase")
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": 1481912143502870,
"parent_rev": "5b5d0eaf76f1b70edd03d075d32bb7d3083162ac", "commit_rev":
"f68cc7007bc5848d35e860339bed213eee0d1d9a"}
Message was sent while issue was closed.
Description was changed from ========== Replaced CursorLoader in PlatformDisplay with a ImageCursor ImageCursor->SetDisplay() is called in Init() so that all cursors are loaded at launch. This fixes the bug where the cursor image is positioned off to the left in Chrome mash. BUG=615861 ========== to ========== Replaced CursorLoader in PlatformDisplay with a ImageCursor ImageCursor->SetDisplay() is called in Init() so that all cursors are loaded at launch. This fixes the bug where the cursor image is positioned off to the left in Chrome mash. BUG=615861 Review-Url: https://codereview.chromium.org/2268563003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Replaced CursorLoader in PlatformDisplay with a ImageCursor ImageCursor->SetDisplay() is called in Init() so that all cursors are loaded at launch. This fixes the bug where the cursor image is positioned off to the left in Chrome mash. BUG=615861 Review-Url: https://codereview.chromium.org/2268563003 ========== to ========== Replaced CursorLoader in PlatformDisplay with a ImageCursor ImageCursor->SetDisplay() is called in Init() so that all cursors are loaded at launch. This fixes the bug where the cursor image is positioned off to the left in Chrome mash. BUG=615861 Committed: https://crrev.com/41725e3dd88eadb07f8a1297c0f8f6ecb9b30c79 Cr-Commit-Position: refs/heads/master@{#439154} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/41725e3dd88eadb07f8a1297c0f8f6ecb9b30c79 Cr-Commit-Position: refs/heads/master@{#439154}
Message was sent while issue was closed.
On 2016/12/16 18:29:14, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/41725e3dd88eadb07f8a1297c0f8f6ecb9b30c79 > Cr-Commit-Position: refs/heads/master@{#439154} Oh awesome! Thanks for getting back to this! Do you know what/when the problem with skia was resolved?
Message was sent while issue was closed.
On 2016/12/16 18:31:12, sadrul wrote: > On 2016/12/16 18:29:14, commit-bot: I haz the power wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/41725e3dd88eadb07f8a1297c0f8f6ecb9b30c79 > > Cr-Commit-Position: refs/heads/master@{#439154} > > Oh awesome! Thanks for getting back to this! Do you know what/when the problem > with skia was resolved? I am not 100% sure but I think it was when this was fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=642879 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
