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

Issue 1827323002: Take screenshot of active window (Closed)

Created:
4 years, 9 months ago by Qiang(Joe) Xu
Modified:
4 years, 8 months ago
Reviewers:
afakhry, Mark P, oshima, sky
CC:
chromium-reviews, asvitkine+watch_chromium.org, kalyank, sadrul, arv+watch_chromium.org, oshima+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Take screenshot of active window window screenshot is bind to shortcut ctrl+alt+f5 BUG=481059 TEST=tested on veyron_jerry chromebook, works as expected. Committed: https://crrev.com/9e787b24578666d11ee57117d821d60aa60665c6 Cr-Commit-Position: refs/heads/master@{#387711}

Patch Set 1 #

Total comments: 13

Patch Set 2 : add a missing file accelerator_controller.cc, changes based on sky's review #

Patch Set 3 : change in chrome_screenshot_grabber.cc to make it match TakeScreenshot API #

Patch Set 4 : changes on calling TakeScreenshot #

Total comments: 6

Patch Set 5 : Merge #

Patch Set 6 : #

Total comments: 2

Patch Set 7 : change none active window early return pos #

Total comments: 1

Patch Set 8 : two test cases about taking active window screenshot #

Total comments: 2

Patch Set 9 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -0 lines) Patch
M ash/accelerators/accelerator_controller.cc View 1 2 3 4 5 6 7 8 3 chunks +14 lines, -0 lines 0 comments Download
M ash/accelerators/accelerator_controller_unittest.cc View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M ash/accelerators/accelerator_table.h View 1 chunk +1 line, -0 lines 0 comments Download
M ash/accelerators/accelerator_table.cc View 1 5 chunks +6 lines, -0 lines 0 comments Download
M ash/screenshot_delegate.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ash/test/test_screenshot_delegate.h View 3 chunks +6 lines, -0 lines 0 comments Download
M ash/test/test_screenshot_delegate.cc View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/app/chromeos_strings.grdp View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/resources/chromeos/keyboard_overlay_data.js View 1 2 3 4 1 chunk +1 line, -0 lines 1 comment Download
M chrome/browser/ui/ash/chrome_screenshot_grabber.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/ash/chrome_screenshot_grabber.cc View 1 2 3 4 7 1 chunk +23 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M tools/metrics/actions/actions.xml View 1 2 3 4 5 2 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 54 (19 generated)
Qiang(Joe) Xu
4 years, 9 months ago (2016-03-24 23:19:04 UTC) #3
sky
https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerator_controller.cc#newcode466 ash/accelerators/accelerator_controller.cc:466: if (screenshot_delegate->CanTakeScreenshot()) Early out if current_window is null? Also, ...
4 years, 9 months ago (2016-03-25 15:27:53 UTC) #5
Qiang(Joe) Xu
attn: add a file which was missing yesterday. https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerator_controller.cc#newcode466 ash/accelerators/accelerator_controller.cc:466: if ...
4 years, 9 months ago (2016-03-25 19:09:41 UTC) #7
sky
https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc#newcode371 chrome/browser/ui/ash/chrome_screenshot_grabber.cc:371: gfx::Rect root_rect = root_window->bounds(); On 2016/03/25 19:09:40, Qiang (Joe) ...
4 years, 9 months ago (2016-03-25 20:37:09 UTC) #8
Mark P
actions.xml lgtm https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actions.xml File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actions.xml#newcode709 tools/metrics/actions/actions.xml:709: <owner>Please list the metric's owners. Add more ...
4 years, 9 months ago (2016-03-25 21:05:02 UTC) #9
Qiang(Joe) Xu
change in chrome_screenshot_grabber.cc based on sky@'s review https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc#newcode372 chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, ...
4 years, 9 months ago (2016-03-25 21:15:55 UTC) #10
sky
https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc#newcode372 chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path); On 2016/03/25 21:15:54, Qiang (Joe) Xu ...
4 years, 9 months ago (2016-03-25 22:58:43 UTC) #11
Qiang(Joe) Xu
On 2016/03/25 22:58:43, sky wrote: > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc > File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrome_screenshot_grabber.cc#newcode372 > ...
4 years, 8 months ago (2016-03-28 17:46:00 UTC) #13
oshima
On 2016/03/28 17:46:00, Qiang (Joe) Xu wrote: > On 2016/03/25 22:58:43, sky wrote: > > ...
4 years, 8 months ago (2016-03-28 17:53:51 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/60001
4 years, 8 months ago (2016-03-30 17:21:04 UTC) #16
Qiang(Joe) Xu
changes in chrome_screenshot_grabber.cc only, the other two file deltas coming from Merge.
4 years, 8 months ago (2016-03-30 17:36:31 UTC) #17
Qiang(Joe) Xu
On 2016/03/28 17:46:00, Qiang (Joe) Xu wrote: > On 2016/03/25 22:58:43, sky wrote: > > ...
4 years, 8 months ago (2016-03-30 17:38:11 UTC) #18
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 8 months ago (2016-03-30 18:33:55 UTC) #20
Qiang(Joe) Xu
On 2016/03/30 18:33:55, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
4 years, 8 months ago (2016-04-13 19:18:00 UTC) #22
oshima
https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accelerator_controller_unittest.cc File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accelerator_controller_unittest.cc#newcode863 ash/accelerators/accelerator_controller_unittest.cc:863: EXPECT_EQ(2, delegate->handle_take_screenshot_count()); shouldn't this be EXPECT_EQ(1, and handle_take_window_screen_screenshot_count()) ? ...
4 years, 8 months ago (2016-04-13 23:05:37 UTC) #23
Qiang(Joe) Xu
https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accelerator_controller_unittest.cc File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accelerator_controller_unittest.cc#newcode863 ash/accelerators/accelerator_controller_unittest.cc:863: EXPECT_EQ(2, delegate->handle_take_screenshot_count()); On 2016/04/13 23:05:36, oshima wrote: > shouldn't ...
4 years, 8 months ago (2016-04-13 23:36:16 UTC) #25
oshima
lgtm
4 years, 8 months ago (2016-04-13 23:40:23 UTC) #26
sky
https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accelerator_controller.cc#newcode468 ash/accelerators/accelerator_controller.cc:468: if (screenshot_delegate->CanTakeScreenshot()) Why do we need to route taking ...
4 years, 8 months ago (2016-04-13 23:56:16 UTC) #27
Qiang(Joe) Xu
https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accelerator_controller.cc#newcode468 ash/accelerators/accelerator_controller.cc:468: if (screenshot_delegate->CanTakeScreenshot()) On 2016/04/13 23:56:16, sky wrote: > Why ...
4 years, 8 months ago (2016-04-14 22:37:53 UTC) #28
sky
Having all this logic in chrome seems wrong, but that is separate. LGTM
4 years, 8 months ago (2016-04-15 00:02:38 UTC) #29
oshima
On 2016/04/15 00:02:38, sky wrote: > Having all this logic in chrome seems wrong, but ...
4 years, 8 months ago (2016-04-15 00:44:26 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827323002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/120001
4 years, 8 months ago (2016-04-15 01:01:41 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/155043)
4 years, 8 months ago (2016-04-15 01:38:08 UTC) #35
Qiang(Joe) Xu
On 2016/04/15 01:38:08, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 years, 8 months ago (2016-04-15 02:42:11 UTC) #36
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827323002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/140001
4 years, 8 months ago (2016-04-15 02:42:38 UTC) #38
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 8 months ago (2016-04-15 03:27:29 UTC) #40
oshima
Or, you can create an active window, which allow you to test two cases with ...
4 years, 8 months ago (2016-04-15 04:37:42 UTC) #41
sky
I don't understand that change. Seems to me the delegate, ChromeScreenShotGrabber, should be able to ...
4 years, 8 months ago (2016-04-15 16:10:59 UTC) #42
Qiang(Joe) Xu
On 2016/04/15 16:10:59, sky wrote: > I don't understand that change. Seems to me the ...
4 years, 8 months ago (2016-04-15 17:11:38 UTC) #43
sky
LGTM https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accelerator_controller.cc#newcode464 ash/accelerators/accelerator_controller.cc:464: DCHECK(screenshot_delegate); nit: move this above 468, right before ...
4 years, 8 months ago (2016-04-15 19:28:24 UTC) #44
Qiang(Joe) Xu
https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accelerator_controller.cc File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accelerator_controller.cc#newcode464 ash/accelerators/accelerator_controller.cc:464: DCHECK(screenshot_delegate); On 2016/04/15 19:28:23, sky wrote: > nit: move ...
4 years, 8 months ago (2016-04-15 21:15:38 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/180001
4 years, 8 months ago (2016-04-15 21:16:43 UTC) #48
commit-bot: I haz the power
Committed patchset #9 (id:180001)
4 years, 8 months ago (2016-04-15 22:03:37 UTC) #50
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/9e787b24578666d11ee57117d821d60aa60665c6 Cr-Commit-Position: refs/heads/master@{#387711}
4 years, 8 months ago (2016-04-15 22:04:47 UTC) #52
afakhry
4 years, 8 months ago (2016-04-16 01:25:12 UTC) #54
Message was sent while issue was closed.
https://codereview.chromium.org/1827323002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/keyboard_overlay_data.js (right):

https://codereview.chromium.org/1827323002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/keyboard_overlay_data.js:15898: 'switch
window<>ALT<>CTRL': 'keyboardOverlayScreenshotWindow',
You need to add a corresponding entry for when the top row keys are treated as
function keys.

That is, you need to add the following entry:
'f5<>ALT<>CTRL<>SEARCH': 'keyboardOverlayScreenshotWindow',

here:
https://chromium.googlesource.com/chromium/src/+/b447a543958befb1e16ade0b3e00...

Powered by Google App Engine
This is Rietveld 408576698