Change snasphotting API to take in a bounds Rect

This replaces the snapshotting code in the pixel tests with the code used to get screenshots for reporting issues.
Added a bounds parameter to window_snapshot.cc so that we can take a picture of just the tab contents and not have to rebaseline with chrome ui changes.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111155

[pkotwicz, 2011-11-11 00:30:42]

Only tested with GTK so far, so please only review that part for now.

[jonathan.backer, 2011-11-11 14:41:20]

I like the general direction. It makes way more sense than using the
thumbnailing code.

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
File chrome/browser/gpu_pixel_browsertest.cc (right):

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
chrome/browser/gpu_pixel_browsertest.cc:260: #define MAYBE_WEBGLTeapot
DISABLED_WebGLTeapot
nit: case is wrong s/WEB/Web

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
chrome/browser/gpu_pixel_browsertest.cc:293: DCHECK(!screenshot_size.IsEmpty());
ASSERT_TRUE

http://codereview.chromium.org/8523022/diff/1/chrome/browser/ui/window_snapsh...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/8523022/diff/1/chrome/browser/ui/window_snapsh...
chrome/browser/ui/window_snapshot/window_snapshot.h:26: // into a byte vector.
Returns the image bounds.
Document that an empty rect means grab the whole region?

Better yet... factor out the commonality between these two functions into a
function in an anonymous namespace in each of the platform specific impls.

http://codereview.chromium.org/8523022/diff/1/chrome/test/data/gpu/webgl_teap...
File chrome/test/data/gpu/webgl_teapot/teapot_files/demo.js (right):

http://codereview.chromium.org/8523022/diff/1/chrome/test/data/gpu/webgl_teap...
chrome/test/data/gpu/webgl_teapot/teapot_files/demo.js:394: epilogue.src =
"./teapot_files/epilogue.js";
Are we missing this in chrome/test/data/gpu/webgl_teapot/teapot_files?

[kkania, 2011-11-11 17:06:31]

I don't remember exactly why we used the thumbnail system before instead of os
screenshots. How are we guaranteed that the right bits will be on the screen
before we take a snapshot?

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
File chrome/browser/gpu_pixel_browsertest.cc (right):

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
chrome/browser/gpu_pixel_browsertest.cc:294: DCHECK(!screenshot_png.empty());
ASSERT_TRUE here too. It's generally better to fail the test than crash. If it
crashes, it will be marked as disabled and will not run. If it fails nicely, it
will be marked as FLAKY or FAILS, which allows the test to still run and get
coverage.

http://codereview.chromium.org/8523022/diff/1/chrome/browser/gpu_pixel_browse...
chrome/browser/gpu_pixel_browsertest.cc:302: DCHECK(success);
ASSERT_TRUE

http://codereview.chromium.org/8523022/diff/1/chrome/browser/ui/window_snapsh...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/8523022/diff/1/chrome/browser/ui/window_snapsh...
chrome/browser/ui/window_snapshot/window_snapshot.h:29: gfx::Rect
snapshot_bounds);
const gfx::Rect& snapshot_bounds

http://codereview.chromium.org/8523022/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8523022/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:3925:
'browser/ui/window_snapshot/window_snapshot.cc',
cc comes before h

[pkotwicz, 2011-11-12 02:05:51]

Tested on Windows and Mac.

Changed teapot.html so that it would be less flaky.

Otherwise, changes as requested

[jonathan.backer, 2011-11-14 13:45:02]

Just nits. Thanks for doing this. There were a lot of platforms to test.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:17: gfx::Rect
GetWindowBounds(gfx::NativeWindow window_handle) {
There are two functions in ui/base/x11_util.h that you could use in place of
this function:

GetX11WindowFromGtkWidget and
GetWindowRect

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:59: GdkWindow*
gdk_window = GTK_WIDGET(window_handle)->window;
GetX11WindowFromGtkWidget

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:60: Display* display =
GDK_WINDOW_XDISPLAY(gdk_window);
GetXDisplay?

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_mac.mm (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:14: namespace browser {
Maybe move this below? No sense nesting an anonymous namespace.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:50: return
GrabWindowSnapshot(window, png_representation, CGRectNull);
Maybe document that CGRectNull means to grab the whole window.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:52: NSScreen* screen =
[[NSScreen screens] objectAtIndex:0];
Bad merge?

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:17: namespace {
No need to nest anon namespace. Customary to follow with a blank line.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:23: return
gfx::Rect(content_rect.right - content_rect.left,
Feel free to ignore, but aren't you really returning a gfx::Size?

[pkotwicz, 2011-11-14 14:42:03]

I realized that I haven't answered your question kkania. I am not sure if we are
guaranteed to see all the right bits on the screen. From my understanding of the
graphics pipeline at the moment that the webpage tells us that all of the
textures are loaded, we still need to notify the browser that it needs to paint
the pixels from WebKit, and the message loop, needs to execute that paint. I
have added a delay, via waiting for epilogue.js to load. I am not sure whether
this delay is guaranteed to be long enough. However, it does mirror the webgl
tests in Webkit and it does work on the dsektop. I am not sure if anyone has
suggestions on how to do this in more rigorous way

[sky, 2011-11-14 16:23:09]

You should get erg to review gtk side and thakis to review mac.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot.h:22: std::vector<unsigned
char>* png_representation);
Can we remove this one in favor of the new one?

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot.h:29: const gfx::Rect
snapshot_bounds);
const gfx::Rect&

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:17: gfx::Rect
GetWindowBounds(gfx::NativeWindow window_handle) {
nit: newline between 16 and 17.

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_mac.mm (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:19: CGRect windowRect)
{
const CGRect&

[pkotwicz, 2011-11-15 15:56:11]

Separated issue into two patches as I am having problems displaying the correct
pixels with --use-gl=osmesa

Changes as requested. Removed old method
GrabWindowSnapshot now always takes in a rect.

Erg, can you please look at window_snapshot_gtk.cc
Thakis, can you please review window_snapshot_mac.cc

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:60: Display* display =
GDK_WINDOW_XDISPLAY(gdk_window);
It seems as if we are trying to deprecate this method. But its a good method.

[sky, 2011-11-15 16:58:08]

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/webui/bug...
File chrome/browser/ui/webui/bug_report_ui.cc (right):

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/webui/bug...
chrome/browser/ui/webui/bug_report_ui.cc:145: window_bounds);
Don't you need to convert window_bounds to have an origin of 0x0?

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot.h:24: const gfx::Rect
snapshot_bounds);
const gfx::Rect&

[Nico, 2011-11-15 17:10:08]

mac bits lgtm if you checked that bug report screenshots still work correctly

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/webui/bug...
File chrome/browser/ui/webui/bug_report_ui.cc (left):

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/webui/bug...
chrome/browser/ui/webui/bug_report_ui.cc:143:
browser::GrabWindowSnapshot(native_window, last_screenshot_png));
nit: I liked the old interface better for the use case in this file. I would've
left the old interface intact, added a new function for your use case, and then
implemented the old one in terms of the new one. That's just me, though :-)

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot_mac.mm (right):

http://codereview.chromium.org/8523022/diff/20001/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:33:
DCHECK(screen_snapshot_bounds.right() <= window_bounds.right());
DCHECK_LE. Also below.

[Elliot Glaysher, 2011-11-15 18:06:34]

gtk lgtm

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/8523022/diff/7002/chrome/browser/ui/window_sna...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:60: Display* display =
GDK_WINDOW_XDISPLAY(gdk_window);
On 2011/11/15 15:56:12, pkotwicz1 wrote:
> It seems as if we are trying to deprecate this method. But its a good method.

Please don't use GetXDisplay(). We aren't guaranteed to have only a single
Display per process, especially with sshing around.

[pkotwicz, 2011-11-15 20:53:26]

Tested on windows, linux and mac.

Changes as requested

[sky, 2011-11-15 21:45:46]

LGTM