Support GrabWindowSnapshot with no g_browser_process

Support calling GrabWindowSnapshot when no BrowserProcess instance is avaliable (eg. tests)


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

[qfel, 2012-07-31 17:22:13]

It'd nice if some BrowserProcess instance was setup during the tests so we
wouldn't need to modify regular code just to support tests. If there's an
elegant way to do that, let me know.

sergeyu: why did u disable FullTabModeIE_RefreshMshtmlTest? It seems to work:
http://build.chromium.org/p/tryserver.chromium/builders/win_cf/builds/313

[grt (UTC plus 2), 2012-07-31 19:50:18]

On 2012/07/31 17:22:13, qfel wrote:
> It'd nice if some BrowserProcess instance was setup during the tests so we
> wouldn't need to modify regular code just to support tests. If there's an
> elegant way to do that, let me know.

I think it'd be cleaner to provide an internal interface to the snapshot code
that can be called from tests.  This function doesn't need to do any policy
checking since it's not available to Chrome, and it doesn't make any sense for
the Chrome Frame tests to spin up a BrowserProcess since they aren't acting like
a browser.

> sergeyu: why did u disable FullTabModeIE_RefreshMshtmlTest? It seems to work:
> http://build.chromium.org/p/tryserver.chromium/builders/win_cf/builds/313

The Chrome Frame test harness takes a snapshot when a test times out so that GCF
devs can look at the screen to get clues as to what went wrong (I can't tell you
how helpful this has been).  The two tests that were disabled didn't themselves
do anything in particular with the snapshot code, they just happened to time out
(and therefore trigger a snapshot) after this CL landed.

[sky, 2012-07-31 23:38:42]

On Tue, Jul 31, 2012 at 12:50 PM,  <grt@chromium.org> wrote:
> On 2012/07/31 17:22:13, qfel wrote:
>>
>> It'd nice if some BrowserProcess instance was setup during the tests so we
>> wouldn't need to modify regular code just to support tests. If there's an
>> elegant way to do that, let me know.
>
>
> I think it'd be cleaner to provide an internal interface to the snapshot
> code
> that can be called from tests.  This function doesn't need to do any policy
> checking since it's not available to Chrome, and it doesn't make any sense
> for
> the Chrome Frame tests to spin up a BrowserProcess since they aren't acting
> like
> a browser.

Said function could be moved to the ui directory.

  -Scott

>
>> sergeyu: why did u disable FullTabModeIE_RefreshMshtmlTest? It seems to
>> work:
>> http://build.chromium.org/p/tryserver.chromium/builders/win_cf/builds/313
>
>
> The Chrome Frame test harness takes a snapshot when a test times out so that
> GCF
> devs can look at the screen to get clues as to what went wrong (I can't tell
> you
> how helpful this has been).  The two tests that were disabled didn't
> themselves
> do anything in particular with the snapshot code, they just happened to time
> out
> (and therefore trigger a snapshot) after this CL landed.
>
> http://codereview.chromium.org/10821104/

[qfel, 2012-08-01 08:34:53]

> Said function could be moved to the ui directory.
Do you mean to move implementations of GrabWindowSnapshotImpl to toplevel
ui/[window_snapshot] directory, and rename it ui::GrabWindowSnapshot? So there
would be two versions (ui:: and chrome::, chrome calling ui after policy
checking)

[grt (UTC plus 2), 2012-08-01 10:07:27]

On 2012/08/01 08:34:53, qfel wrote:
> > Said function could be moved to the ui directory.
> Do you mean to move implementations of GrabWindowSnapshotImpl to toplevel
> ui/[window_snapshot] directory, and rename it ui::GrabWindowSnapshot? So there
> would be two versions (ui:: and chrome::, chrome calling ui after policy
> checking)

That sounds reasonable to me.  Be sure to include documentation for both methods
indicating when the chrome:: variant must be used (i.e., anything triggered in
the browser, I imagine).

[qfel, 2012-08-01 13:17:10]

Split GrabWindowSnapshot.

CC ben: I have no idea why (only) window_snapshot_win.cc was excluded from build
when ash=1. Since I moved the file, I had to move the exclude to ui.gyp, but I'm
not sure that was the right thing to do.

[grt (UTC plus 2), 2012-08-01 13:39:04]

lgtm.  please send up a tryjob.  thanks.

[qfel, 2012-08-01 13:56:29]

> lgtm.  please send up a tryjob.  thanks.

There are tryjobs on patch set #2 (win_cf may fail, patch set #3 fixes it). I
suppose there's no need to re-run them all on #3, given the minimal change?
win_cf for #3 is still pending, so I presume that's why it's not listed (yet)
here.

[grt (UTC plus 2), 2012-08-01 13:59:25]

On 2012/08/01 13:56:29, qfel wrote:
> > lgtm.  please send up a tryjob.  thanks.
> 
> There are tryjobs on patch set #2 (win_cf may fail, patch set #3 fixes it). I
> suppose there's no need to re-run them all on #3, given the minimal change?
> win_cf for #3 is still pending, so I presume that's why it's not listed (yet)
> here.

ah, i see.  rock on.

[sky, 2012-08-01 15:49:36]

LGTM

http://codereview.chromium.org/10821104/diff/3002/ui/window_snapshot/window_s...
File ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10821104/diff/3002/ui/window_snapshot/window_s...
ui/window_snapshot/window_snapshot.h:34: #endif  //
CHROME_BROWSER_UI_WINDOW_SNAPSHOT_WINDOW_SNAPSHOT_H_
Update this too.

[commit-bot: I haz the power, 2012-08-02 08:15:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10821104/11019

[commit-bot: I haz the power, 2012-08-02 08:15:36]

Failed to apply patch for ui/ui.gyp:
While running patch -p1 --forward --force;
patching file ui/ui.gyp
Hunk #1 succeeded at 467 with fuzz 1 (offset 3 lines).
Hunk #2 succeeded at 512 (offset 8 lines).
Hunk #3 FAILED at 553.
1 out of 3 hunks FAILED -- saving rejects to file ui/ui.gyp.rej

[commit-bot: I haz the power, 2012-08-02 08:29:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10821104/7040

[commit-bot: I haz the power, 2012-08-02 09:52:25]

Change committed as 149604

[Joao da Silva, 2012-08-02 10:33:54]

This broke the win aura build.

[qfel, 2012-08-02 11:53:48]

window_snapshot_aura.cc uses compositor, and so aura build with shared library
breaks on Windows. On the other hand, GrabWindowSnapshot is supposed to work on
every platform, so moving it to aura (which uses compositor) is a no-no.
I could move it back to the browser and rename, but since it does not use
browser code at all, I'm not fully happy with such solution (though it's what is
used to be).
Any better suggestions anyone?

[sky, 2012-08-02 15:08:05]

You could make the actual snapshot function settable so that aura
could set it. Screen does similar things. That said, I'm fine with
moving this back to chrome and using a friend to get at the internal
function for testing. Probably less error prone that way.

  -Scott

On Thu, Aug 2, 2012 at 4:53 AM,  <qfel@google.com> wrote:
> window_snapshot_aura.cc uses compositor, and so aura build with shared
> library
> breaks on Windows. On the other hand, GrabWindowSnapshot is supposed to work
> on
> every platform, so moving it to aura (which uses compositor) is a no-no.
> I could move it back to the browser and rename, but since it does not use
> browser code at all, I'm not fully happy with such solution (though it's
> what is
> used to be).
> Any better suggestions anyone?
>
> https://chromiumcodereview.appspot.com/10821104/