Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2167613003:
Hide mouse cursor when screenshot is taking (Closed)
|
Created:
4 years, 5 months ago by Qiang(Joe) Xu Modified:
4 years, 5 months ago Reviewers:
sky CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide mouse cursor when screenshot is taking
BUG=597270
TEST=device test saw bug away
Committed: https://crrev.com/eb93d50aef7c4924b00d53c900208de7d604414c
Cr-Commit-Position: refs/heads/master@{#407325}
Patch Set 1 #Patch Set 2 : check nullptr #
Total comments: 2
Patch Set 3 : show the cursor when screenshot completed #
Total comments: 2
Patch Set 4 : ScopedCursorHide to manage the hide/show #
Total comments: 6
Patch Set 5 : based on last cr comments #
Total comments: 4
Patch Set 6 : nit adjustment #Messages
Total messages: 38 (26 generated)
The CQ bit was checked by warx@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by warx@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.
Description was changed from ========== Hide mouse cursor when screenshot is taking BUG=597270 ========== to ========== Hide mouse cursor when screenshot is taking BUG=597270 TEST=device test saw bug away ==========
warx@chromium.org changed reviewers: + sky@chromium.org
Hi sky@, could you help review on this? tks!
https://codereview.chromium.org/2167613003/diff/20001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/20001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:142: cursor_client->HideCursor(); Aren't you missing a corresonding Show?
The CQ bit was checked by warx@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.
showing the cursor when screenshot is completed. https://codereview.chromium.org/2167613003/diff/20001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/20001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:142: cursor_client->HideCursor(); On 2016/07/21 15:31:42, sky wrote: > Aren't you missing a corresonding Show? Yes. It is missing. I add it now.
https://codereview.chromium.org/2167613003/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:383: shell->cursor_manager()->ShowCursor(); Why is the show here and not in ScreenshotGrabber? I would expect the place doing the hide to also do the show. To do otherwise is error prone. Also please put the calls to show/hide in the same object that is easy to reason about lifetime. See ScopedCursorHider in aura/window for what I mean.
https://codereview.chromium.org/2167613003/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:383: shell->cursor_manager()->ShowCursor(); On 2016/07/21 19:40:26, sky wrote: > Why is the show here and not in ScreenshotGrabber? I would expect the place > doing the hide to also do the show. To do otherwise is error prone. Also please > put the calls to show/hide in the same object that is easy to reason about > lifetime. See ScopedCursorHider in aura/window for what I mean. Thanks, based on the suggestion I got a updated version.
The CQ bit was checked by warx@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.
https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:117: if (!window_->IsRootWindow()) This should be a DCHECK. https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:119: aura::client::CursorClient* cursor_client = Having to have all these checks in both the constructor and destructor is ugly. How about a static function that returns a std::unique_ptr<ScopedCursorHider> only if appropriate? The constructor is private. That way you get rid of hid_cursor. https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.h (right): https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.h:83: std::unique_ptr<ScopedCursorHider> cursor_hider_; members go after functions (see style guide).
The CQ bit was checked by warx@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.
Hi, sky@ I updated based on the comments. Thanks! https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:117: if (!window_->IsRootWindow()) On 2016/07/22 16:14:44, sky wrote: > This should be a DCHECK. Done. https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:119: aura::client::CursorClient* cursor_client = On 2016/07/22 16:14:44, sky wrote: > Having to have all these checks in both the constructor and destructor is ugly. > How about a static function that returns a std::unique_ptr<ScopedCursorHider> > only if appropriate? The constructor is private. That way you get rid of > hid_cursor. done. Thanks for suggestion. https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.h (right): https://codereview.chromium.org/2167613003/diff/60001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.h:83: std::unique_ptr<ScopedCursorHider> cursor_hider_; On 2016/07/22 16:14:44, sky wrote: > members go after functions (see style guide). Done.
LGTM https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:115: static std::unique_ptr<ScopedCursorHider> Get(aura::Window* window) { nit: Get->Create, with a suitable description that it may return null. https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:187: if (cursor_hider_) You don't need the conditional here, cursor_hider_.reset() always works.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2167613003/#ps100001 (title: "nit adjustment")
https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... File ui/snapshot/screenshot_grabber.cc (right): https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:115: static std::unique_ptr<ScopedCursorHider> Get(aura::Window* window) { On 2016/07/22 23:35:04, sky wrote: > nit: Get->Create, with a suitable description that it may return null. Done. https://codereview.chromium.org/2167613003/diff/80001/ui/snapshot/screenshot_... ui/snapshot/screenshot_grabber.cc:187: if (cursor_hider_) On 2016/07/22 23:35:04, sky wrote: > You don't need the conditional here, cursor_hider_.reset() always works. Done.
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 ========== Hide mouse cursor when screenshot is taking BUG=597270 TEST=device test saw bug away ========== to ========== Hide mouse cursor when screenshot is taking BUG=597270 TEST=device test saw bug away ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Hide mouse cursor when screenshot is taking BUG=597270 TEST=device test saw bug away ========== to ========== Hide mouse cursor when screenshot is taking BUG=597270 TEST=device test saw bug away Committed: https://crrev.com/eb93d50aef7c4924b00d53c900208de7d604414c Cr-Commit-Position: refs/heads/master@{#407325} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/eb93d50aef7c4924b00d53c900208de7d604414c Cr-Commit-Position: refs/heads/master@{#407325} |
