Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827323002:
Take screenshot of active window (Closed)
|
Created:
4 years, 9 months ago by Qiang(Joe) Xu Modified:
4 years, 8 months ago 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. |
DescriptionTake 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
Messages
Total messages: 54 (19 generated)
Description was changed from ========== Take screenshot of active window For issue 481059 BUG= ========== to ========== Take screenshot of active window For crbug/481059 window screenshot is bind to shortcut ctrl+alt+f5 BUG= ==========
warx@chromium.org changed reviewers: + mpearson@chromium.org, sky@chromium.org, stevenjb@chromium.org
Description was changed from ========== Take screenshot of active window For crbug/481059 window screenshot is bind to shortcut ctrl+alt+f5 BUG= ========== to ========== Take screenshot of active window window screenshot is bind to shortcut ctrl+alt+f5 BUG=481059 ==========
https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:466: if (screenshot_delegate->CanTakeScreenshot()) Early out if current_window is null? Also, current_window is a bit misleading as it's more of active_window. Although just window is fine by me too. https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:371: gfx::Rect root_rect = root_window->bounds(); This code assumes window is a direct child of root_window, that is not necessary. https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path); I'm confused by the coordinates here. You're passing in window to get the screenshot, but you're passing in bounds in terms of another window. I would expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it that? If you do need root_rect in terms of the root_window, then you should convert to the root_window instead of assuming window->parent() == root_window. https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:709: <owner>Please list the metric's owners. Add more owner tags as needed.</owner> HA! You are suppose to fill this in!
Description was changed from ========== Take screenshot of active window window screenshot is bind to shortcut ctrl+alt+f5 BUG=481059 ========== to ========== 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. ==========
attn: add a file which was missing yesterday. https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:466: if (screenshot_delegate->CanTakeScreenshot()) On 2016/03/25 15:27:53, sky wrote: > Early out if current_window is null? Also, current_window is a bit misleading as > it's more of active_window. Although just window is fine by me too. I added: if (!active_window) return; once this is added, AcceleratorControllerTest.GlobalAccelerators EXPECT_EQ(1, delegate->handle_take_window_screenshot_count()); --> will fail (unable to terminate process group), I temporarily remove this test. https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:371: gfx::Rect root_rect = root_window->bounds(); On 2016/03/25 15:27:53, sky wrote: > This code assumes window is a direct child of root_window, that is not > necessary. GetRootWindow() will get root window, it is not parent() method which only gets direct parent. https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path); On 2016/03/25 15:27:53, sky wrote: > I'm confused by the coordinates here. You're passing in window to get the > screenshot, but you're passing in bounds in terms of another window. I would > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it that? > > If you do need root_rect in terms of the root_window, then you should convert to > the root_window instead of assuming window->parent() == root_window. Here we should use the root window's coordinates as we looked at TakeScreenshot method, if we are using window->bounds() rect, it will use the rect based on the window's rect, which will cause the problem in crbug/481059 comment＃20. We should use fullscreen's rect instead, in this case, that is root window. window->parent() doesn't sound right, https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... I believe GetRootWindow is doing the right way, clarify me if I am wrong on this. https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:709: <owner>Please list the metric's owners. Add more owner tags as needed.</owner> On 2016/03/25 15:27:53, sky wrote: > HA! You are suppose to fill this in! done. It is interesting that most of them just uses the template...
https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... 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) Xu wrote: > On 2016/03/25 15:27:53, sky wrote: > > This code assumes window is a direct child of root_window, that is not > > necessary. > > GetRootWindow() will get root window, it is not parent() method which only gets > direct parent. GetRootWindow() walks up the tree starting at window. So, window is not necessarily a direct child of the root. https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path); On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > On 2016/03/25 15:27:53, sky wrote: > > I'm confused by the coordinates here. You're passing in window to get the > > screenshot, but you're passing in bounds in terms of another window. I would > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it that? > > > > If you do need root_rect in terms of the root_window, then you should convert > to > > the root_window instead of assuming window->parent() == root_window. > > Here we should use the root window's coordinates as we looked at TakeScreenshot > method, if we are using window->bounds() rect, it will use the rect based on the > window's rect, which will cause the problem in crbug/481059 comment＃20. We > should use fullscreen's rect instead, in this case, that is root window. > > window->parent() doesn't sound right, > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > I believe GetRootWindow is doing the right way, clarify me if I am wrong on > this. This is the comment says for the function you're using: // Takes a screenshot of |rect| in |window| in that window's coordinate space // saving the result to |screenshot_path|. So again, I would expect you to use gfx::Rect(window->bounds().size()); As to the root window. If you care about the location in terms of the root, then you should be using ConvertRectToTarget().
actions.xml lgtm https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:709: <owner>Please list the metric's owners. Add more owner tags as needed.</owner> On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > On 2016/03/25 15:27:53, sky wrote: > > HA! You are suppose to fill this in! > > done. It is interesting that most of them just uses the template... Most of these actions predate the existence of a descriptions file. When we created the file, we put in all those actions as unowned and undescribed. Sadly, not that many people have claimed them.
change in chrome_screenshot_grabber.cc based on sky@'s review https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:372: screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path); On 2016/03/25 20:37:08, sky wrote: > On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > > On 2016/03/25 15:27:53, sky wrote: > > > I'm confused by the coordinates here. You're passing in window to get the > > > screenshot, but you're passing in bounds in terms of another window. I would > > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it > that? > > > > > > If you do need root_rect in terms of the root_window, then you should > convert > > to > > > the root_window instead of assuming window->parent() == root_window. > > > > Here we should use the root window's coordinates as we looked at > TakeScreenshot > > method, if we are using window->bounds() rect, it will use the rect based on > the > > window's rect, which will cause the problem in crbug/481059 comment＃20. We > > should use fullscreen's rect instead, in this case, that is root window. > > > > window->parent() doesn't sound right, > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > > > I believe GetRootWindow is doing the right way, clarify me if I am wrong on > > this. > > This is the comment says for the function you're using: > // Takes a screenshot of |rect| in |window| in that window's coordinate space > // saving the result to |screenshot_path|. > So again, I would expect you to use gfx::Rect(window->bounds().size()); > > As to the root window. If you care about the location in terms of the root, then > you should be using ConvertRectToTarget(). I get it: My original one is: passing active_window and root_rect now is: passing root_window and active_window_rect I am using window->GetBoundsInRootWindow() // Returns the window's bounds in root window's coordinates. this change matches well with the TakeScreenshot API.
https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... 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 wrote: > On 2016/03/25 20:37:08, sky wrote: > > On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > > > On 2016/03/25 15:27:53, sky wrote: > > > > I'm confused by the coordinates here. You're passing in window to get the > > > > screenshot, but you're passing in bounds in terms of another window. I > would > > > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it > > that? > > > > > > > > If you do need root_rect in terms of the root_window, then you should > > convert > > > to > > > > the root_window instead of assuming window->parent() == root_window. > > > > > > Here we should use the root window's coordinates as we looked at > > TakeScreenshot > > > method, if we are using window->bounds() rect, it will use the rect based on > > the > > > window's rect, which will cause the problem in crbug/481059 comment＃20. We > > > should use fullscreen's rect instead, in this case, that is root window. > > > > > > window->parent() doesn't sound right, > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > > > > > I believe GetRootWindow is doing the right way, clarify me if I am wrong on > > > this. > > > > This is the comment says for the function you're using: > > // Takes a screenshot of |rect| in |window| in that window's coordinate > space > > // saving the result to |screenshot_path|. > > So again, I would expect you to use gfx::Rect(window->bounds().size()); > > > > As to the root window. If you care about the location in terms of the root, > then > > you should be using ConvertRectToTarget(). > > I get it: > My original one is: passing active_window and root_rect > now is: passing root_window and active_window_rect > I am using window->GetBoundsInRootWindow() // Returns the window's bounds in > root window's coordinates. > this change matches well with the TakeScreenshot API. I don't think that's right either. The problem with doing that is that if another window happens to be on top of the window you're taking a screenshot of it'll be included in the output. I don't think that is expected.
warx@chromium.org changed reviewers: + oshima@chromium.org
On 2016/03/25 22:58:43, sky wrote: > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > 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 wrote: > > On 2016/03/25 20:37:08, sky wrote: > > > On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > > > > On 2016/03/25 15:27:53, sky wrote: > > > > > I'm confused by the coordinates here. You're passing in window to get > the > > > > > screenshot, but you're passing in bounds in terms of another window. I > > would > > > > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't it > > > that? > > > > > > > > > > If you do need root_rect in terms of the root_window, then you should > > > convert > > > > to > > > > > the root_window instead of assuming window->parent() == root_window. > > > > > > > > Here we should use the root window's coordinates as we looked at > > > TakeScreenshot > > > > method, if we are using window->bounds() rect, it will use the rect based > on > > > the > > > > window's rect, which will cause the problem in crbug/481059 comment＃20. We > > > > should use fullscreen's rect instead, in this case, that is root window. > > > > > > > > window->parent() doesn't sound right, > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > > > > > > > I believe GetRootWindow is doing the right way, clarify me if I am wrong > on > > > > this. > > > > > > This is the comment says for the function you're using: > > > // Takes a screenshot of |rect| in |window| in that window's coordinate > > space > > > // saving the result to |screenshot_path|. > > > So again, I would expect you to use gfx::Rect(window->bounds().size()); > > > > > > As to the root window. If you care about the location in terms of the root, > > then > > > you should be using ConvertRectToTarget(). > > > > I get it: > > My original one is: passing active_window and root_rect > > now is: passing root_window and active_window_rect > > I am using window->GetBoundsInRootWindow() // Returns the window's bounds in > > root window's coordinates. > > this change matches well with the TakeScreenshot API. > > I don't think that's right either. The problem with doing that is that if > another window happens to be on top of the window you're taking a screenshot of > it'll be included in the output. I don't think that is expected. I discussed with Oshima. We want a different approach to implement this, which is described as: entering the window screenshot session, mouse hover to the window that wants to be screenshoted and click it to finish the screenshot. I am working on it and will submit once it is ready.
On 2016/03/28 17:46:00, Qiang (Joe) Xu wrote: > On 2016/03/25 22:58:43, sky wrote: > > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > > File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): > > > > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > > 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 wrote: > > > On 2016/03/25 20:37:08, sky wrote: > > > > On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > > > > > On 2016/03/25 15:27:53, sky wrote: > > > > > > I'm confused by the coordinates here. You're passing in window to get > > the > > > > > > screenshot, but you're passing in bounds in terms of another window. I > > > would > > > > > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't > it > > > > that? > > > > > > > > > > > > If you do need root_rect in terms of the root_window, then you should > > > > convert > > > > > to > > > > > > the root_window instead of assuming window->parent() == root_window. > > > > > > > > > > Here we should use the root window's coordinates as we looked at > > > > TakeScreenshot > > > > > method, if we are using window->bounds() rect, it will use the rect > based > > on > > > > the > > > > > window's rect, which will cause the problem in crbug/481059 comment＃20. > We > > > > > should use fullscreen's rect instead, in this case, that is root window. > > > > > > > > > > window->parent() doesn't sound right, > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > > > > > > > > > I believe GetRootWindow is doing the right way, clarify me if I am wrong > > on > > > > > this. > > > > > > > > This is the comment says for the function you're using: > > > > // Takes a screenshot of |rect| in |window| in that window's coordinate > > > space > > > > // saving the result to |screenshot_path|. > > > > So again, I would expect you to use gfx::Rect(window->bounds().size()); > > > > > > > > As to the root window. If you care about the location in terms of the > root, > > > then > > > > you should be using ConvertRectToTarget(). > > > > > > I get it: > > > My original one is: passing active_window and root_rect > > > now is: passing root_window and active_window_rect > > > I am using window->GetBoundsInRootWindow() // Returns the window's bounds in > > > root window's coordinates. > > > this change matches well with the TakeScreenshot API. > > > > I don't think that's right either. The problem with doing that is that if > > another window happens to be on top of the window you're taking a screenshot > of > > it'll be included in the output. I don't think that is expected. > > I discussed with Oshima. We want a different approach to implement this, which > is described as: entering the window screenshot session, mouse hover to the > window that wants to be screenshoted and click it to finish the screenshot. > I am working on it and will submit once it is ready. And sky@ is correct. It should pass that window to the grabber with full window rect.
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/patch-status/1827323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/60001
changes in chrome_screenshot_grabber.cc only, the other two file deltas coming from Merge.
On 2016/03/28 17:46:00, Qiang (Joe) Xu wrote: > On 2016/03/25 22:58:43, sky wrote: > > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > > File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): > > > > > https://codereview.chromium.org/1827323002/diff/1/chrome/browser/ui/ash/chrom... > > 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 wrote: > > > On 2016/03/25 20:37:08, sky wrote: > > > > On 2016/03/25 19:09:40, Qiang (Joe) Xu wrote: > > > > > On 2016/03/25 15:27:53, sky wrote: > > > > > > I'm confused by the coordinates here. You're passing in window to get > > the > > > > > > screenshot, but you're passing in bounds in terms of another window. I > > > would > > > > > > expect root_rect to be gfx::Rect(window->bounds().size()); Why isn't > it > > > > that? > > > > > > > > > > > > If you do need root_rect in terms of the root_window, then you should > > > > convert > > > > > to > > > > > > the root_window instead of assuming window->parent() == root_window. > > > > > > > > > > Here we should use the root window's coordinates as we looked at > > > > TakeScreenshot > > > > > method, if we are using window->bounds() rect, it will use the rect > based > > on > > > > the > > > > > window's rect, which will cause the problem in crbug/481059 comment＃20. > We > > > > > should use fullscreen's rect instead, in this case, that is root window. > > > > > > > > > > window->parent() doesn't sound right, > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > > > > > > > > > > I believe GetRootWindow is doing the right way, clarify me if I am wrong > > on > > > > > this. > > > > > > > > This is the comment says for the function you're using: > > > > // Takes a screenshot of |rect| in |window| in that window's coordinate > > > space > > > > // saving the result to |screenshot_path|. > > > > So again, I would expect you to use gfx::Rect(window->bounds().size()); > > > > > > > > As to the root window. If you care about the location in terms of the > root, > > > then > > > > you should be using ConvertRectToTarget(). > > > > > > I get it: > > > My original one is: passing active_window and root_rect > > > now is: passing root_window and active_window_rect > > > I am using window->GetBoundsInRootWindow() // Returns the window's bounds in > > > root window's coordinates. > > > this change matches well with the TakeScreenshot API. > > > > I don't think that's right either. The problem with doing that is that if > > another window happens to be on top of the window you're taking a screenshot > of > > it'll be included in the output. I don't think that is expected. > > I discussed with Oshima. We want a different approach to implement this, which > is described as: entering the window screenshot session, mouse hover to the > window that wants to be screenshoted and click it to finish the screenshot. > I am working on it and will submit once it is ready. Stick with taking active window screenshot, see crbug/481059
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
warx@chromium.org changed reviewers: - stevenjb@chromium.org
On 2016/03/30 18:33:55, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. ping: oshima/sky, could you review on this ash/ and chrome/ again when you have time? tks.
https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accele... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accele... 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()) ? https://codereview.chromium.org/1827323002/diff/60001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1827323002/diff/60001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:4780: </message> sort the order. (move after IDS_KEYBOARD_OVERLAY_SCREENSHOT_REGION) https://codereview.chromium.org/1827323002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12076: <description>User initiates an active window screenshot.</description> "a window screenshot" I'll add a window selection per kuscher@'s request.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accele... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/1827323002/diff/60001/ash/accelerators/accele... 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 this be > > EXPECT_EQ(1, and handle_take_window_screen_screenshot_count()) > > ? Done. https://codereview.chromium.org/1827323002/diff/60001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1827323002/diff/60001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:4780: </message> On 2016/04/13 23:05:37, oshima wrote: > sort the order. (move after IDS_KEYBOARD_OVERLAY_SCREENSHOT_REGION) Done. https://codereview.chromium.org/1827323002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1827323002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12076: <description>User initiates an active window screenshot.</description> On 2016/04/13 23:05:37, oshima wrote: > "a window screenshot" > > I'll add a window selection per kuscher@'s request. Done.
lgtm
https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accel... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accel... ash/accelerators/accelerator_controller.cc:468: if (screenshot_delegate->CanTakeScreenshot()) Why do we need to route taking the screenshot to the delegate?
https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accel... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/120001/ash/accelerators/accel... ash/accelerators/accelerator_controller.cc:468: if (screenshot_delegate->CanTakeScreenshot()) On 2016/04/13 23:56:16, sky wrote: > Why do we need to route taking the screenshot to the delegate? Because currently taking screenshot has dependencies on chrome/browser side, like generating file name depending on Profile, saving to file_manager, and send notifications.
Having all this logic in chrome seems wrong, but that is separate. LGTM
On 2016/04/15 00:02:38, sky wrote: > Having all this logic in chrome seems wrong, but that is separate. LGTM I agree. Part of it was historical when we used FILE thread to save files, and other parts has heavy dependency to Profile/notifications, which we may be able to refactor them now. I'm hoping that we can clean up these delegates when migrating to mash.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1827323002/#ps120001 (title: " ")
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2016/04/15 01:38:08, commit-bot: I haz the power wrote: > 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_...) Oh, I remember this failure. Sorry about that. It is because of non active window case early return in HandleTakeActiveWindowScreenshot so that there is no call to delegate. Move the early return in chrome_screenshot_grabber.cc will be fine. Since I made changes to the code, sky@ how this looks to you?
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/patch-status/1827323002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827323002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Or, you can create an active window, which allow you to test two cases with and without active window. Since this will change a bit later, I don't have strong opinion about it and will defer to sky@. https://codereview.chromium.org/1827323002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right): https://codereview.chromium.org/1827323002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_screenshot_grabber.cc:362: return; move this to the top.
I don't understand that change. Seems to me the delegate, ChromeScreenShotGrabber, should be able to assume window is non-null. On Thu, Apr 14, 2016 at 7:42 PM, <warx@chromium.org> wrote: > On 2016/04/15 01:38:08, commit-bot: I haz the power wrote: >> 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_...) > > Oh, I remember this failure. Sorry about that. > It is because of non active window case early return in > HandleTakeActiveWindowScreenshot so that there is no call to delegate. Move > the > early return in chrome_screenshot_grabber.cc will be fine. > > Since I made changes to the code, sky@ how this looks to you? > > https://codereview.chromium.org/1827323002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/15 16:10:59, sky wrote: > I don't understand that change. Seems to me the delegate, > ChromeScreenShotGrabber, should be able to assume window is non-null. > > On Thu, Apr 14, 2016 at 7:42 PM, <mailto:warx@chromium.org> wrote: > > On 2016/04/15 01:38:08, commit-bot: I haz the power wrote: > >> 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_...) > > > > Oh, I remember this failure. Sorry about that. > > It is because of non active window case early return in > > HandleTakeActiveWindowScreenshot so that there is no call to delegate. Move > > the > > early return in chrome_screenshot_grabber.cc will be fine. > > > > Since I made changes to the code, sky@ how this looks to you? > > > > https://codereview.chromium.org/1827323002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > This is one way to make right of test counter of window screenshot, which is to fire call anyway w/wo active window. I agreed with oshima. We can create an active window do test both cases. New patch is ready.
LGTM https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accel... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accel... ash/accelerators/accelerator_controller.cc:464: DCHECK(screenshot_delegate); nit: move this above 468, right before you use it.
https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accel... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/1827323002/diff/160001/ash/accelerators/accel... ash/accelerators/accelerator_controller.cc:464: DCHECK(screenshot_delegate); On 2016/04/15 19:28:23, sky wrote: > nit: move this above 468, right before you use it. Done.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, oshima@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1827323002/#ps180001 (title: " ")
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
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9e787b24578666d11ee57117d821d60aa60665c6 Cr-Commit-Position: refs/heads/master@{#387711}
Message was sent while issue was closed.
afakhry@chromium.org changed reviewers: + afakhry@chromium.org
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... |
