Take screenshot of active window

chrome/browser/ui/ash/chrome_screenshot_grabber.cc

Index: chrome/browser/ui/ash/chrome_screenshot_grabber.cc
diff --git a/chrome/browser/ui/ash/chrome_screenshot_grabber.cc b/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
index fff9cd3e48b83ea80a120bb3fed304a49de177f2..20225126af58f941d52c7086453729ae08cc4bcd 100644
--- a/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
+++ b/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
@@ -350,6 +350,29 @@ void ChromeScreenshotGrabber::HandleTakePartialScreenshot(
   content::RecordAction(base::UserMetricsAction("Screenshot_TakePartial"));
 }
 
+void ChromeScreenshotGrabber::HandleTakeWindowScreenshot(aura::Window* window) {
+  if (ScreenshotsDisabled()) {
+    screenshot_grabber_->NotifyScreenshotCompleted(
+        ui::ScreenshotGrabberObserver::SCREENSHOTS_DISABLED, base::FilePath());
+    return;
+  }
+
+  base::FilePath screenshot_directory;
+  if (!GetScreenshotDirectory(&screenshot_directory)) {
+    screenshot_grabber_->NotifyScreenshotCompleted(
+        ui::ScreenshotGrabberObserver::SCREENSHOT_GET_DIR_FAILED,
+        base::FilePath());
+    return;
+  }
+
+  base::FilePath screenshot_path =
+      screenshot_directory.AppendASCII(GetScreenshotBaseFilename() + ".png");
+  aura::Window* root_window = window->GetRootWindow();
+  gfx::Rect root_rect = root_window->bounds();

[sky, 2016/03/25 15:27:53]

This code assumes window is a direct child of root_window, that is not
necessary.

[Qiang(Joe) Xu, 2016/03/25 19:09:40]

GetRootWindow() will get root window, it is not parent() method which only gets
direct parent.

[sky, 2016/03/25 20:37:08]

GetRootWindow() walks up the tree starting at window. So, window is not
necessarily a direct child of the root.

+  screenshot_grabber_->TakeScreenshot(window, root_rect, screenshot_path);

[sky, 2016/03/25 15:27:53]

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.

[Qiang(Joe) Xu, 2016/03/25 19:09:40]

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.

[sky, 2016/03/25 20:37:08]

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().

[Qiang(Joe) Xu, 2016/03/25 21:15:54]

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.

[sky, 2016/03/25 22:58:43]

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.

+  content::RecordAction(base::UserMetricsAction("Screenshot_TakeWindow"));
+}
+
 bool ChromeScreenshotGrabber::CanTakeScreenshot() {
   return screenshot_grabber_->CanTakeScreenshot();
 }