Implemented GetWindowSnapshot on RenderViewImpl

Implemented GetWindowSnapshot on RenderViewImpl

This necessitated the relocation of the previous chrome::GrabWindowSnapshot code
to ui/snapshot, which has been turned into it's own component to avoid circular dependencies with aura.
A new variant of GrabWindowSnapshot, GrabViewSnapshot, has been added as well to facilitate easier usage by views. chrome::GrabWindowSnapshotForUser was left in place to accomodate existing calls
to the API, but now calls up to the ui/snapshot code.

This is a subset of the prior CL 11362023, which has been broken apart to facilitate easier reviews

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

[Ken Russell (switch to Gerrit), 2012-11-08 22:27:44]

Basically LGTM but there is one bug you need to fix.

https://codereview.chromium.org/11399002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/11399002/diff/1/content/common/view_messages....
content/common/view_messages.h:1935: // Requests a snapshot of the given window
Add "." here and below.

https://codereview.chromium.org/11399002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11399002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:1720: int id =
next_smooth_scroll_gesture_id_++;
Typo: next_smooth_scroll_gesture_id_ -> next_snapshot_id_.

[jam, 2012-11-09 02:21:29]

just nits and one question:

it looks like the the moved code doesn't  have any dependencies on content. I
wonder if ui/base would be a better place.

Ben: what do you think?

https://codereview.chromium.org/11399002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11399002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.cc:2041:
std::vector<unsigned char> png;
nit: 2 space tabbing per style guide

https://codereview.chromium.org/11399002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/11399002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.h:577: void
OnMsgGetWindowSnapshot(const int id);
just OnGetWindowSnapshot is good enough (I realize some places put OnMsg, but
that's redundant)

https://codereview.chromium.org/11399002/diff/1/content/browser/window_util_a...
File content/browser/window_util_aura.cc (right):

https://codereview.chromium.org/11399002/diff/1/content/browser/window_util_a...
content/browser/window_util_aura.cc:57: }  // namespace chrome
nit: update

https://codereview.chromium.org/11399002/diff/1/content/browser/window_util_m...
File content/browser/window_util_mac_unittest.mm (right):

https://codereview.chromium.org/11399002/diff/1/content/browser/window_util_m...
content/browser/window_util_mac_unittest.mm:23: namespace chrome {
update the namespace

https://codereview.chromium.org/11399002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/11399002/diff/1/content/common/view_messages....
content/common/view_messages.h:1937: int      /* message id */)
nit: here and below, just one space before start of comment per convention

https://codereview.chromium.org/11399002/diff/1/content/common/view_messages....
content/common/view_messages.h:1942: gfx::Size /* image dimensions */,
nit: here and in the rest of the lines you added, the convention is that the
parameter name in the dispatching function is used inside the comment

https://codereview.chromium.org/11399002/diff/1/content/public/browser/window...
File content/public/browser/window_util.h (right):

https://codereview.chromium.org/11399002/diff/1/content/public/browser/window...
content/public/browser/window_util.h:12: class PrefService;
nuke

https://codereview.chromium.org/11399002/diff/1/content/public/browser/window...
content/public/browser/window_util.h:20: // Like
chrome::GrabWindowSnapshotForUser, but does not perform additional
content shouldn't refer to chrome even in comments

[bajones, 2012-11-09 18:02:51]

Nits addressed. Sorry, still getting acclimated to the code style!

[Ben Goodger (Google), 2012-11-12 18:23:16]

the location seems fine to me.

[commit-bot: I haz the power, 2012-11-12 18:26:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/10001

[commit-bot: I haz the power, 2012-11-12 18:27:01]

Presubmit check for 11399002-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host
    content/browser
    content
    chrome
    content/public/browser
    content/renderer
    chrome/browser/ui
    content/common/view_messages.h

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[bajones, 2012-11-12 18:37:18]

Could I get a thumbs up and/or further comments from the directory owners?

[jamesr, 2012-11-12 18:41:15]

lgtm

http://codereview.chromium.org/11399002/diff/10001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/11399002/diff/10001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1720: void
RenderViewImpl::OnWindowSnapshotCompleted(const int snapshot_id,
we don't typically use const for primitive parameters - it doesn't mean much
since the function gets a copy of the value anyway

[jamesr, 2012-11-12 18:41:39]

On 2012/11/12 18:41:15, jamesr wrote:
> lgtm

for content/renderer/, that is.  I have no powers elsewhere.

[Ben Goodger (Google), 2012-11-12 21:07:34]

jam and I agreed ui/base would be a good place for this.

[jam, 2012-11-12 21:09:26]

On 2012/11/12 21:07:34, Ben Goodger (Google) wrote:
> jam and I agreed ui/base would be a good place for this.

Brandon: sorry I originally told you content/public and content/browser, but I
didn't look closely enough to realize that this code doesn't have any
dependencies on content. Since it's generic code that doesn't depend on content,
ui/base is preferable. The reason is that generic directories like base or ui
are where we move code that's used by multiple modules (i.e. content and
chrome).

[bajones, 2012-11-14 21:52:49]

The snapshot files have been relocated to ui/base as requested.

[jam, 2012-11-15 01:05:58]

content lgtm

https://codereview.chromium.org/11399002/diff/14003/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11399002/diff/14003/content/common/view_messa...
content/common/view_messages.h:1937:
IPC_MESSAGE_ROUTED3(ViewMsg_WindowSnapshotCompleted,
nit: this should go with the rest of the browser->renderer message above this
section (i.e. line 1432)

https://codereview.chromium.org/11399002/diff/14003/ui/base/snapshot/snapshot...
File ui/base/snapshot/snapshot_mac_unittest.mm (right):

https://codereview.chromium.org/11399002/diff/14003/ui/base/snapshot/snapshot...
ui/base/snapshot/snapshot_mac_unittest.mm:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
you'll need to move this from chrome's test gypi to the gypi in src/ui

[commit-bot: I haz the power, 2012-11-26 21:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/16001

[commit-bot: I haz the power, 2012-11-26 21:10:45]

Presubmit check for 11399002-16001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui
    content/common/view_messages.h

Presubmit checks took 2.2s to calculate.

[bajones, 2012-11-26 21:18:19]

Looks like I still need a security review on view_messages.h and an LGTM from
ben@ or sky@ for the ui/ changes.

[Tom Sepez, 2012-11-26 21:25:47]

Won't this become a security issue once we move to a world with proper process
isolation (i.e. out-of-process iframes)?

[jschuh, 2012-11-26 21:31:00]

On 2012/11/26 21:25:47, Tom Sepez wrote:
> Won't this become a security issue once we move to a world with proper process
> isolation (i.e. out-of-process iframes)?

From past discussion I seem to remember that we said this was okay from a
security perspective if the browser gated the screenshot IPC behind a
command-line switch.

[bajones, 2012-11-26 21:36:12]

On 2012/11/26 21:31:00, Justin Schuh wrote:
> On 2012/11/26 21:25:47, Tom Sepez wrote:
> > Won't this become a security issue once we move to a world with proper
process
> > isolation (i.e. out-of-process iframes)?
> 
> From past discussion I seem to remember that we said this was okay from a
> security perspective if the browser gated the screenshot IPC behind a
> command-line switch.

It's not in this CL, but in a follow-up CL the API that utilizes these functions
will be gated behind the --enable-gpu-benchmarking switch.

[Tom Sepez, 2012-11-26 21:37:35]

Ah.  It's behind kEnableGPUBenchmarking. Two things:

RenderViewHostImpl::OnGetWindowSnapshot() probably should have a comment saying
that this is behind a flag because it is unsafe so that down the road, someone
isn't tempted to take it out from behind the flag.

kEnableGPUBenchmarking isn't one of the big scary flags in bad_flags_prompt.cc
that triggers the "stability and security may suffer" warning.  If this is
opening a hole, then it should probably trip this warning.

[Tom Sepez, 2012-11-26 21:38:30]

Otherwise, messages LGTM apart from those comments.

[Ben Goodger (Google), 2012-11-27 17:36:38]

Nice move. LGTM.

[commit-bot: I haz the power, 2012-11-27 18:06:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/24001

[commit-bot: I haz the power, 2012-11-27 18:27:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 01:25:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/22002

[commit-bot: I haz the power, 2012-11-28 02:09:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 18:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/24006

[commit-bot: I haz the power, 2012-11-28 19:02:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 05:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/22074

[commit-bot: I haz the power, 2012-11-29 06:19:25]

Retried try job too often on win_aura for step(s) aura_unittests,
content_browsertests, content_unittests, views_unittests

[bajones, 2012-11-30 22:15:31]

Previous patch set had circular dependency issues in the Aura implementation
that weren't seen until it hit the trybots. Latest patch set (#11) fixes it.
Side effect is that aura builds will not be able to use snapshot in telemetry
until an alternate implementation can be created.

I'd like one last LGTM before I push this through. Thanks!

[Ken Russell (switch to Gerrit), 2012-11-30 22:32:54]

LGTM again. Nice work pushing this through. Couple of minor comments.

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2037: void
RenderViewHostImpl::OnGetWindowSnapshot(const int id) {
id -> snapshot_id

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2041: // because it poses
security concerns and should only be used for testing
Please add period at end of comment.

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/browser/re...
content/browser/renderer_host/render_view_host_impl.h:570: void
OnGetWindowSnapshot(const int id);
id -> snapshot_id

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/11399002/diff/18087/content/common/vie...
content/common/view_messages.h:1951: int /* message id */)
For clarity, change "message id" to "snapshot_id". Also, @jam's indicated to me
that the arguments of the message handler should match the names in these
comments.

[jam, 2012-11-30 23:07:18]

On 2012/11/30 22:15:31, bajones wrote:
> Previous patch set had circular dependency issues in the Aura implementation
> that weren't seen until it hit the trybots. Latest patch set (#11) fixes it.
> Side effect is that aura builds will not be able to use snapshot in telemetry
> until an alternate implementation can be created.

what about the solution that was discussed yesterday to get this data based on
the root native view?

win_aura is the future for windows, so not supporting aura now means that this
will get broken when we switch to aura.

[Ken Russell (switch to Gerrit), 2012-11-30 23:10:51]

On 2012/11/30 23:07:18, John Abd-El-Malek wrote:
> On 2012/11/30 22:15:31, bajones wrote:
> > Previous patch set had circular dependency issues in the Aura implementation
> > that weren't seen until it hit the trybots. Latest patch set (#11) fixes it.
> > Side effect is that aura builds will not be able to use snapshot in
telemetry
> > until an alternate implementation can be created.
> 
> what about the solution that was discussed yesterday to get this data based on
> the root native view?
> 
> win_aura is the future for windows, so not supporting aura now means that this
> will get broken when we switch to aura.

After discussion with piman, we do not want to take the risk of breaking
features like feedback on Chrome OS by changing the semantics of the snapshots
they take. ui::base's snapshot functionality reads the (possibly inconsistent)
front buffer; GrabAuraCompositorSnapshot reads the (always consistent) back
buffer.

@bajones indicated he will look into implementing Android and Aura versions of
the front-buffer snapshot. However, we want to land the initial patch to be able
to work on other things (building tests on this functionality) in parallel.

[jam, 2012-12-01 00:16:07]

On 2012/11/30 23:10:51, kbr wrote:
> On 2012/11/30 23:07:18, John Abd-El-Malek wrote:
> > On 2012/11/30 22:15:31, bajones wrote:
> > > Previous patch set had circular dependency issues in the Aura
implementation
> > > that weren't seen until it hit the trybots. Latest patch set (#11) fixes
it.
> > > Side effect is that aura builds will not be able to use snapshot in
> telemetry
> > > until an alternate implementation can be created.
> > 
> > what about the solution that was discussed yesterday to get this data based
on
> > the root native view?
> > 
> > win_aura is the future for windows, so not supporting aura now means that
this
> > will get broken when we switch to aura.
> 
> After discussion with piman, we do not want to take the risk of breaking
> features like feedback on Chrome OS by changing the semantics of the snapshots
> they take. 

feeback is not a high priority feature. if it was broken, which I doubt would
happen, then it wouldn't be bad.

I'm a bit concerned that this leaves the code in a confusing state in the
meantime.

Is it really a lot of effort to try the approach above now?

> ui::base's snapshot functionality reads the (possibly inconsistent)
> front buffer; GrabAuraCompositorSnapshot reads the (always consistent) back
> buffer.
> 
> @bajones indicated he will look into implementing Android and Aura versions of
> the front-buffer snapshot. However, we want to land the initial patch to be
able
> to work on other things (building tests on this functionality) in parallel.

[Ken Russell (switch to Gerrit), 2012-12-01 00:26:57]

On 2012/12/01 00:16:07, John Abd-El-Malek wrote:
> On 2012/11/30 23:10:51, kbr wrote:
> > On 2012/11/30 23:07:18, John Abd-El-Malek wrote:
> > > On 2012/11/30 22:15:31, bajones wrote:
> > > > Previous patch set had circular dependency issues in the Aura
> implementation
> > > > that weren't seen until it hit the trybots. Latest patch set (#11) fixes
> it.
> > > > Side effect is that aura builds will not be able to use snapshot in
> > telemetry
> > > > until an alternate implementation can be created.
> > > 
> > > what about the solution that was discussed yesterday to get this data
based
> on
> > > the root native view?
> > > 
> > > win_aura is the future for windows, so not supporting aura now means that
> this
> > > will get broken when we switch to aura.
> > 
> > After discussion with piman, we do not want to take the risk of breaking
> > features like feedback on Chrome OS by changing the semantics of the
snapshots
> > they take. 
> 
> feeback is not a high priority feature. if it was broken, which I doubt would
> happen, then it wouldn't be bad.

+piman

I think Chrome OS team members would disagree with you. I am personally
concerned about regressions in this area.

> I'm a bit concerned that this leaves the code in a confusing state in the
> meantime.
> 
> Is it really a lot of effort to try the approach above now?

Yes, it is. I think we should allow the implementation of the new screenshot API
for Telemetry to proceed in parallel to development of Android and Aura ports of
the front-buffer snapshot API.

> > ui::base's snapshot functionality reads the (possibly inconsistent)
> > front buffer; GrabAuraCompositorSnapshot reads the (always consistent) back
> > buffer.
> > 
> > @bajones indicated he will look into implementing Android and Aura versions
of
> > the front-buffer snapshot. However, we want to land the initial patch to be
> able
> > to work on other things (building tests on this functionality) in parallel.

[jam, 2012-12-01 00:33:40]

On 2012/12/01 00:26:57, kbr wrote:
> On 2012/12/01 00:16:07, John Abd-El-Malek wrote:
> > On 2012/11/30 23:10:51, kbr wrote:
> > > On 2012/11/30 23:07:18, John Abd-El-Malek wrote:
> > > > On 2012/11/30 22:15:31, bajones wrote:
> > > > > Previous patch set had circular dependency issues in the Aura
> > implementation
> > > > > that weren't seen until it hit the trybots. Latest patch set (#11)
fixes
> > it.
> > > > > Side effect is that aura builds will not be able to use snapshot in
> > > telemetry
> > > > > until an alternate implementation can be created.
> > > > 
> > > > what about the solution that was discussed yesterday to get this data
> based
> > on
> > > > the root native view?
> > > > 
> > > > win_aura is the future for windows, so not supporting aura now means
that
> > this
> > > > will get broken when we switch to aura.
> > > 
> > > After discussion with piman, we do not want to take the risk of breaking
> > > features like feedback on Chrome OS by changing the semantics of the
> snapshots
> > > they take. 
> > 
> > feeback is not a high priority feature. if it was broken, which I doubt
would
> > happen, then it wouldn't be bad.
> 
> +piman
> 
> I think Chrome OS team members would disagree with you. I am personally
> concerned about regressions in this area.

There are hundreds of engineers touching the code that ships on ChromeOS.
Regressions happen all the time. That's why we have canaries, dev and beta
builds. What is the likelihood that it's implemented as above, verified locally
on cros, and then breaks in the field?

> 
> > I'm a bit concerned that this leaves the code in a confusing state in the
> > meantime.
> > 
> > Is it really a lot of effort to try the approach above now?
> 
> Yes, it is. I think we should allow the implementation of the new screenshot
API
> for Telemetry to proceed in parallel to development of Android and Aura ports
of
> the front-buffer snapshot API.

My concern is that now there are two similar functions, one in ui/base and one
in chrome. it's not clear to an engineer wanting to use one which one to call.
it's also not clear that calling the one in ui/base doesn't work on aura.

It seems that the work can proceed in parallel if someone patches this change
locally.

> 
> > > ui::base's snapshot functionality reads the (possibly inconsistent)
> > > front buffer; GrabAuraCompositorSnapshot reads the (always consistent)
back
> > > buffer.
> > > 
> > > @bajones indicated he will look into implementing Android and Aura
versions
> of
> > > the front-buffer snapshot. However, we want to land the initial patch to
be
> > able
> > > to work on other things (building tests on this functionality) in
parallel.

[Ken Russell (switch to Gerrit), 2012-12-01 03:22:03]

On 2012/12/01 00:33:40, John Abd-El-Malek wrote:
> On 2012/12/01 00:26:57, kbr wrote:
> > On 2012/12/01 00:16:07, John Abd-El-Malek wrote:
> > > On 2012/11/30 23:10:51, kbr wrote:
> > > > On 2012/11/30 23:07:18, John Abd-El-Malek wrote:
> > > > > On 2012/11/30 22:15:31, bajones wrote:
> > > > > > Previous patch set had circular dependency issues in the Aura
> > > implementation
> > > > > > that weren't seen until it hit the trybots. Latest patch set (#11)
> fixes
> > > it.
> > > > > > Side effect is that aura builds will not be able to use snapshot in
> > > > telemetry
> > > > > > until an alternate implementation can be created.
> > > > > 
> > > > > what about the solution that was discussed yesterday to get this data
> > based
> > > on
> > > > > the root native view?
> > > > > 
> > > > > win_aura is the future for windows, so not supporting aura now means
> that
> > > this
> > > > > will get broken when we switch to aura.
> > > > 
> > > > After discussion with piman, we do not want to take the risk of breaking
> > > > features like feedback on Chrome OS by changing the semantics of the
> > snapshots
> > > > they take. 
> > > 
> > > feeback is not a high priority feature. if it was broken, which I doubt
> would
> > > happen, then it wouldn't be bad.
> > 
> > +piman
> > 
> > I think Chrome OS team members would disagree with you. I am personally
> > concerned about regressions in this area.
> 
> There are hundreds of engineers touching the code that ships on ChromeOS.
> Regressions happen all the time. That's why we have canaries, dev and beta
> builds. What is the likelihood that it's implemented as above, verified
locally
> on cros, and then breaks in the field?
> 
> > 
> > > I'm a bit concerned that this leaves the code in a confusing state in the
> > > meantime.
> > > 
> > > Is it really a lot of effort to try the approach above now?
> > 
> > Yes, it is. I think we should allow the implementation of the new screenshot
> API
> > for Telemetry to proceed in parallel to development of Android and Aura
ports
> of
> > the front-buffer snapshot API.
> 
> My concern is that now there are two similar functions, one in ui/base and one
> in chrome. it's not clear to an engineer wanting to use one which one to call.
> it's also not clear that calling the one in ui/base doesn't work on aura.

I think that the documentation should be expanded to be clearer about what the
semantics are and what's implemented on what plaforms, but I don't think it's
feasible to get rid of the chrome:: namespace API all at once. Even if the
ui::base:: (front buffer) version of the API were implemented for Aura then it's
possible a regression in feedback would cause this CL to be reverted, and it
would *not* be OK for the ui::base:: version to be removed once the dependent
CLs have landed.

> It seems that the work can proceed in parallel if someone patches this change
> locally.

No, the other work is blocked on this. Brandon has already implemented the
Telemetry API; we are blocked on its integration into shipping versions of
Chrome to deploy tests to GPU vendors which will prevent graphics driver
regressions in the future.

> > 
> > > > ui::base's snapshot functionality reads the (possibly inconsistent)
> > > > front buffer; GrabAuraCompositorSnapshot reads the (always consistent)
> back
> > > > buffer.
> > > > 
> > > > @bajones indicated he will look into implementing Android and Aura
> versions
> > of
> > > > the front-buffer snapshot. However, we want to land the initial patch to
> be
> > > able
> > > > to work on other things (building tests on this functionality) in
> parallel.

[piman, 2012-12-04 00:05:47]

https://chromiumcodereview.appspot.com/11399002/diff/20008/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11399002/diff/20008/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2052: if
(ui::GrabViewSnapshot(GetView()->GetNativeView(),
Why not delegating to the view here, which would allow platform-specific
handling without going to a layer that's too low and causes ugliness to solve
circular dependencies? E.g. on Aura that'd let you delegate to the Aura layer
that can then to the right thing with the compositor. If you go directly to
ui/base, then you need to provide strange hooks and/or global mapping tables and
overall complexity.

[bajones, 2012-12-12 23:31:10]

Okay, let's try and get this one pushed through once and for all! The dependency
issues with aura have been resolved by moving out of ui/base/snapshot and into
ui/snapshot as suggested by ben@. This is my first foray into defining a
component, so I wouldn't be surprised in the least if I've missed some detail.
Please let me know if there's any issues that need addressing!

[Ken Russell (switch to Gerrit), 2012-12-12 23:32:37]

On 2012/12/12 23:31:10, bajones wrote:
> Okay, let's try and get this one pushed through once and for all! The
dependency
> issues with aura have been resolved by moving out of ui/base/snapshot and into
> ui/snapshot as suggested by ben@. This is my first foray into defining a
> component, so I wouldn't be surprised in the least if I've missed some detail.
> Please let me know if there's any issues that need addressing!

Please send this CL to the try servers. That's the only way issues on all
platforms will be caught.

[Nico, 2012-12-12 23:35:29]

On 2012/12/12 23:31:10, bajones wrote:
> Okay, let's try and get this one pushed through once and for all! The
dependency
> issues with aura have been resolved by moving out of ui/base/snapshot and into
> ui/snapshot as suggested by ben@. This is my first foray into defining a
> component, so I wouldn't be surprised in the least if I've missed some detail.
> Please let me know if there's any issues that need addressing!

Can you update the CL description with what this patch does now? (The
description doesn't say "component" anywhere for example)

[bajones, 2012-12-12 23:39:13]

On 2012/12/12 23:35:29, Nico wrote:
> On 2012/12/12 23:31:10, bajones wrote:
> > Okay, let's try and get this one pushed through once and for all! The
> dependency
> > issues with aura have been resolved by moving out of ui/base/snapshot and
into
> > ui/snapshot as suggested by ben@. This is my first foray into defining a
> > component, so I wouldn't be surprised in the least if I've missed some
detail.
> > Please let me know if there's any issues that need addressing!
> 
> Can you update the CL description with what this patch does now? (The
> description doesn't say "component" anywhere for example)

Done

[Ken Russell (switch to Gerrit), 2012-12-12 23:43:13]

LGTM

https://chromiumcodereview.appspot.com/11399002/diff/33001/ui/snapshot/snapsh...
File ui/snapshot/snapshot_mac.mm (right):

https://chromiumcodereview.appspot.com/11399002/diff/33001/ui/snapshot/snapsh...
ui/snapshot/snapshot_mac.mm:9: #include <iostream>
Unnecessary; please remove.

[Nico, 2012-12-12 23:46:20]

The ui and chrome bits in this change lgtm. The component bits look good too as
far as I can tell.

Someone else will have to lg the content bits.

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2048: // because it poses
security concerns and should only be used for testing.
nit: Might be worth mentioning that if this was code that's actually used in
production chrome, a shared memory segment might be preferable for transferring
the png data.

[piman, 2012-12-12 23:59:50]

LGTM+nits for content/

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2053: gfx::Rect
snapshot_bounds(
nit: this should work:
gfx::Rect snapshot_bounds(view_bounds.size());

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2056:
snapshot_size.SetSize(snapshot_bounds.width(),
nit: snapshot_size = snapshot_bounds.size();
Even better, declare here too.

https://chromiumcodereview.appspot.com/11399002/diff/33001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:2060: &png,
snapshot_bounds)) {
nit: arguments should align in function call:
    if (ui::GrabViewSnapshot(GetView()->GetNativeView(),
                             &png, snapshot_bounds)) {

[jam, 2012-12-13 08:36:21]

content lgtm

[commit-bot: I haz the power, 2012-12-13 21:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/46001

[commit-bot: I haz the power, 2012-12-13 21:30:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ben Goodger (Google), 2012-12-13 21:41:34]

Looks like there's a DEPS file in src/ui, but that's a poor place for one. In
the mean time, define your own minimal DEPS file.

https://codereview.chromium.org/11399002/diff/46001/ui/snapshot/snapshot.gyp
File ui/snapshot/snapshot.gyp (right):

https://codereview.chromium.org/11399002/diff/46001/ui/snapshot/snapshot.gyp#...
ui/snapshot/snapshot.gyp:3: # found in the LICENSE file.
put DEPS in this directory.

[commit-bot: I haz the power, 2012-12-14 18:35:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/45006

[commit-bot: I haz the power, 2012-12-14 18:52:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-14 19:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/51005

[commit-bot: I haz the power, 2012-12-14 22:10:40]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-14 22:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/51005

[commit-bot: I haz the power, 2012-12-15 04:52:29]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 18:51:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11399002/51005

[commit-bot: I haz the power, 2012-12-15 22:20:11]

Change committed as 173329