Introduce XGetImage() for GrabWindowSnapshot() in ChromeOS.

Introduce XGetImage() for GrabWindowSnapshot() in ChromeOS.


BUG=119492
TEST=manually done

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137556

[Daniel Erat, 2012-05-14 22:50:49]

Mind re-uploading?  I just get a bunch of "error: old chunk mismatch" messages
when I try to look at the diffs.

[Jun Mukai, 2012-05-14 22:52:26]

On 2012/05/14 22:50:49, Daniel Erat wrote:
> Mind re-uploading?  I just get a bunch of "error: old chunk mismatch" messages
> when I try to look at the diffs.

uploaded.  Please see patchset3.

[Daniel Erat, 2012-05-14 23:17:40]

https://chromiumcodereview.appspot.com/10386124/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

https://chromiumcodereview.appspot.com/10386124/diff/12001/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:22: #if
defined(OS_CHROMEOS)
Should this be OS_LINUX?  I don't see anything Chrome OS-specific here.

https://chromiumcodereview.appspot.com/10386124/diff/12001/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:23: // We use
XGetImage() for ChromeOS for the performance reason.
nit: s/the performance reason/performance reasons/

Does Windows not have the same performance problems?

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
ui/aura/root_window.h:253: bool GrabWindowSnapshot(std::vector<unsigned char>*
png_representation,
nit: Add a comment describing what this does.

Should you drop "Window" from the method name?  Is there anything specific to a
particular window here, or can the snapshot span multiple windows?

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
ui/aura/root_window_host_linux.cc:819: gfx::PNGCodec::Encode(data,
gfx::PNGCodec::FORMAT_BGRA,
I don't love the inflexibility of doing PNG encoding here (and on the UI thread)
instead of handing back raw data.  Are you mostly doing this so you can call
XFree() here without needing to copy the data?

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
File ui/aura/root_window_host_mac.mm (right):

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
ui/aura/root_window_host_mac.mm:178: return false;
NOTIMPLEMENTED()

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
File ui/aura/root_window_host_win.cc (right):

https://chromiumcodereview.appspot.com/10386124/diff/12001/ui/aura/root_windo...
ui/aura/root_window_host_win.cc:273: NOTIMPLEMENTED();
don't think that this will compile; you need to return false.

[Jun Mukai, 2012-05-15 22:39:24]

http://codereview.chromium.org/10386124/diff/12001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/10386124/diff/12001/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:22: #if
defined(OS_CHROMEOS)
On 2012/05/14 23:17:40, Daniel Erat wrote:
> Should this be OS_LINUX?  I don't see anything Chrome OS-specific here.

Done.

http://codereview.chromium.org/10386124/diff/12001/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:23: // We use
XGetImage() for ChromeOS for the performance reason.
On 2012/05/14 23:17:40, Daniel Erat wrote:
> nit: s/the performance reason/performance reasons/
> 
> Does Windows not have the same performance problems?

Done.

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window.h#newc...
ui/aura/root_window.h:253: bool GrabWindowSnapshot(std::vector<unsigned char>*
png_representation,
On 2012/05/14 23:17:40, Daniel Erat wrote:
> nit: Add a comment describing what this does.
> 
> Should you drop "Window" from the method name?  Is there anything specific to
a
> particular window here, or can the snapshot span multiple windows?

Done.

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:819: gfx::PNGCodec::Encode(data,
gfx::PNGCodec::FORMAT_BGRA,
On 2012/05/14 23:17:40, Daniel Erat wrote:
> I don't love the inflexibility of doing PNG encoding here (and on the UI
thread)
> instead of handing back raw data.  Are you mostly doing this so you can call
> XFree() here without needing to copy the data?

That's why original browser::GrabWindowSnapshot returns PNG format.  Also, the
response format would get unclear if win/mac impls of root_window_host want to
support this method.

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_m...
File ui/aura/root_window_host_mac.mm (right):

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_m...
ui/aura/root_window_host_mac.mm:178: return false;
On 2012/05/14 23:17:40, Daniel Erat wrote:
> NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_w...
File ui/aura/root_window_host_win.cc (right):

http://codereview.chromium.org/10386124/diff/12001/ui/aura/root_window_host_w...
ui/aura/root_window_host_win.cc:273: NOTIMPLEMENTED();
On 2012/05/14 23:17:40, Daniel Erat wrote:
> don't think that this will compile; you need to return false.

Done.

[Daniel Erat, 2012-05-15 23:35:08]

cc marcheu@ to confirm about byte_order

http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:10: #include
"ui/aura/root_window.h"
nit: alphabetize

http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:819: gfx::PNGCodec::Encode(data,
gfx::PNGCodec::FORMAT_BGRA,
I don't think it's safe to assume the image format like this.  In the old X
window manager for Chrome OS, I had to check byte_order to determine whether it
was BGR or RGB:

http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...

[marcheu, 2012-05-16 00:09:04]

On 2012/05/15 23:35:08, Daniel Erat wrote:
> cc marcheu@ to confirm about byte_order
> 
>
http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
> File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):
> 
>
http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
> chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:10: #include
> "ui/aura/root_window.h"
> nit: alphabetize
> 
>
http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
> File ui/aura/root_window_host_linux.cc (right):
> 
>
http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
> ui/aura/root_window_host_linux.cc:819: gfx::PNGCodec::Encode(data,
> gfx::PNGCodec::FORMAT_BGRA,
> I don't think it's safe to assume the image format like this.  In the old X
> window manager for Chrome OS, I had to check byte_order to determine whether
it
> was BGR or RGB:
> 
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...


Yes XImages are endianness-dependent, but maybe PNG encoding does the same thing
and it balances out? (I think it doesn't though)

[Jun Mukai, 2012-05-16 18:07:43]

http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/10386124/diff/13010/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:10: #include
"ui/aura/root_window.h"
On 2012/05/15 23:35:08, Daniel Erat wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10386124/diff/13010/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:819: gfx::PNGCodec::Encode(data,
gfx::PNGCodec::FORMAT_BGRA,
On 2012/05/15 23:35:08, Daniel Erat wrote:
> I don't think it's safe to assume the image format like this.  In the old X
> window manager for Chrome OS, I had to check byte_order to determine whether
it
> was BGR or RGB:
> 
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...

Wrote a simple detector.  Thanks for pointing.

[Daniel Erat, 2012-05-16 19:43:50]

LGTM with one minor issue.

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window_host_linu...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:837: LOG(ERROR) << "bits_per_pixel is too
small";
don't forget to XFree(image) before returning here

[sky, 2012-05-16 21:32:27]

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window.h#newcode248
ui/aura/root_window.h:248: const gfx::Rect& snapshot_bounds);
out params should be last.

[Jun Mukai, 2012-05-16 21:43:57]

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window.h#newcode248
ui/aura/root_window.h:248: const gfx::Rect& snapshot_bounds);
On 2012/05/16 21:32:27, sky wrote:
> out params should be last.

Done.

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window_host_linu...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10386124/diff/12/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:837: LOG(ERROR) << "bits_per_pixel is too
small";
On 2012/05/16 19:43:50, Daniel Erat wrote:
> don't forget to XFree(image) before returning here

Done.

[sky, 2012-05-16 21:48:23]

LGTM