[devtools] Support different encodings for Page.CaptureScreenshot.

[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282
Review-Url: https://codereview.chromium.org/2592983002
Cr-Original-Commit-Position: refs/heads/master@{#445730}
Committed: https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf18410957101a119
Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#446021}
Committed: https://chromium.googlesource.com/chromium/src/+/7233c1a70ea0194caf10f4365cf3520e5be7aa8f

[Eric Seckler, 2016-12-21 11:02:48]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 11:03:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 13:39:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 13:39:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2016-12-21 15:03:23]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2016-12-21 15:03:28]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 15:03:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-12-21 15:10:54]

eseckler@chromium.org changed reviewers:
+ dgozman@chromium.org

[Eric Seckler, 2016-12-21 15:10:54]

Dmitry, would you be up for having a look at this? :)

[Sami, 2016-12-21 16:30:24]

https://codereview.chromium.org/2592983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2592983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:424: { "name":
"data", "type": "string", "description": "Base64-encoded image data (PNG)." }
drive-by nit: png or jpeg

[commit-bot: I haz the power, 2016-12-21 18:14:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 18:14:55]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2016-12-22 16:57:32]

Dmitry, WDYT? :)

https://codereview.chromium.org/2592983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2592983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:424: { "name":
"data", "type": "string", "description": "Base64-encoded image data (PNG)." }
On 2016/12/21 16:30:23, Sami wrote:
> drive-by nit: png or jpeg

Got rid of this, since it should be clear from above parameter.

[pfeldman, 2016-12-22 18:07:40]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-12-22 18:07:41]

https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2358: if
(ui::GrabViewSnapshot(
The idea behind this grab method is that we test the entire stack, unlike when
using CopyFromCompositingSurface. UI/GL people demanded that it was so.

Switching to compositor is probably fine, but is likely to produce a wave of
flakiness on their bots. How about we make GrabViewSnapshot encode
conditionally?

[Eric Seckler, 2016-12-22 19:52:37]

Thanks for taking a look, Pavel :)

On 2016/12/22 18:07:41, pfeldman wrote:
>
https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_impl.cc (left):
> 
>
https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_impl.cc:2358: if
> (ui::GrabViewSnapshot(
> The idea behind this grab method is that we test the entire stack, unlike when
> using CopyFromCompositingSurface. UI/GL people demanded that it was so.
> 
> Switching to compositor is probably fine, but is likely to produce a wave of
> flakiness on their bots. How about we make GrabViewSnapshot encode
> conditionally?
I'm not entirely sure I understand this correctly. Is DevTool's
CaptureScreenshot used on their bots to test the UI/GL stack?

Adding encoding options to ui::GrabViewSnapshot has the disadvantage that they
need to be considered for different architectures separately - at least if we
don't want to convert twice, so seems more complicated. But yes, this would be
alternative - sadly it won't get rid of OS-level interferences in the screenshot
result (e.g. Mac's rounded window borders) -- but from what you say, this may be
intended?

[pfeldman, 2016-12-22 22:56:18]

On 2016/12/22 19:52:37, Eric Seckler wrote:
> Thanks for taking a look, Pavel :)
> 
> On 2016/12/22 18:07:41, pfeldman wrote:
> >
>
https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
> > File content/browser/renderer_host/render_widget_host_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/2592983002/diff/40001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_impl.cc:2358: if
> > (ui::GrabViewSnapshot(
> > The idea behind this grab method is that we test the entire stack, unlike
when
> > using CopyFromCompositingSurface. UI/GL people demanded that it was so.
> > 
> > Switching to compositor is probably fine, but is likely to produce a wave of
> > flakiness on their bots. How about we make GrabViewSnapshot encode
> > conditionally?
> I'm not entirely sure I understand this correctly. Is DevTool's
> CaptureScreenshot used on their bots to test the UI/GL stack?
> 

Yes, remote debugging protocol is used for chrome's automated testing.

> Adding encoding options to ui::GrabViewSnapshot has the disadvantage that they
> need to be considered for different architectures separately - at least if we
> don't want to convert twice, so seems more complicated. But yes, this would be
> alternative - sadly it won't get rid of OS-level interferences in the
screenshot
> result (e.g. Mac's rounded window borders) -- but from what you say, this may
be
> intended?

That's the point of the end-to-end testing.

[Eric Seckler, 2016-12-23 16:20:35]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, it switches RWHImpl from using
ui::GrabViewSnapshot() to capture screenshots to
RWHV::CopyFromCompositingSurface(), which supports grabbing an SkBitmap
instead of png. This change also ensures that rounded window borders on
Mac aren't visible in the resulting screenshot.

BUG=676282
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch adds encoding options to
ui::Grab*Snapshot* methods.

BUG=676282
==========

[Eric Seckler, 2016-12-23 16:21:35]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 16:21:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-12-23 16:25:24]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch adds encoding options to
ui::Grab*Snapshot* methods.

BUG=676282
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch adds encoding options to
ui::Grab*Snapshot* methods.

BUG=676282
==========

[commit-bot: I haz the power, 2016-12-23 16:38:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 16:38:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Eric Seckler, 2016-12-23 16:46:30]

Patchset #4 (id:80001) has been deleted

[Eric Seckler, 2016-12-23 16:47:42]

Patchset #3 (id:60001) has been deleted

[Eric Seckler, 2016-12-23 16:47:49]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 16:48:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 18:55:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 18:55:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-01-03 10:51:52]

Patchset #3 (id:100001) has been deleted

[Eric Seckler, 2017-01-03 10:52:00]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 10:52:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-03 10:55:28]

Thanks for confirming, Pavel. I've moved the encoding into the ui::Grab*Snapshot
methods, PTAL :)

[commit-bot: I haz the power, 2017-01-03 12:26:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 12:26:21]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-01-03 18:42:33]

I think we should grab once and encode multiple times instead of grabbing for
each client, wdyt?

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:665:
content::ExecuteScript(shell()->web_contents()->GetRenderViewHost(),
I wonder if there is a theoretical race here - you are sending this info to the
renderer to be dispatched on main, while the screenshot is read natively in the
browser.

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2368: if
(ui::GrabViewSnapshot(GetView()->GetNativeView(), &data,
Why don't we grab it once and then encode here?

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:887: int
pending_browser_snapshots_;
not sure why you need it now.

[Eric Seckler, 2017-01-03 19:18:09]

Thanks Pavel, comments inline :)

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:665:
content::ExecuteScript(shell()->web_contents()->GetRenderViewHost(),
On 2017/01/03 18:42:33, pfeldman wrote:
> I wonder if there is a theoretical race here - you are sending this info to
the
> renderer to be dispatched on main, while the screenshot is read natively in
the
> browser.

Hmm, I was under the impression that content::ExecuteScript waits for the result
of the script execution?

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2368: if
(ui::GrabViewSnapshot(GetView()->GetNativeView(), &data,
On 2017/01/03 18:42:33, pfeldman wrote:
> Why don't we grab it once and then encode here?

What would that look like? I moved the encoding into GrabViewSnapshot since it
is done differently depending on the platform. Would we instead always encode
into PNG in GrabViewSnapshot and then decode and reencode here for JPEG?

I considered the overhead of that de+reencode to be more significant compared to
the (probably unlikely) case that we have multiple screenshots in the queue for
the same frame - is that a bad assumption? :)

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:887: int
pending_browser_snapshots_;
On 2017/01/03 18:42:33, pfeldman wrote:
> not sure why you need it now.

Currently, OnSnapshotDataReceived can't remove from
pending_browser_snapshot_params_, because it may be called synchronously during
iteration on the map in WindowSnapshotReachedScreen. So I'm removing in
WindowSnapshotReachedScreen instead and using this count to figure out if
OnSnapshotDataReceived should reset the power saver blocker.

[pfeldman, 2017-01-04 00:10:50]

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:665:
content::ExecuteScript(shell()->web_contents()->GetRenderViewHost(),
Oh, you are using the harness one. I looked at the RVH fetch and assumed it was
api call. You could still use following URL to avoid roundtrip:

data:text/html,<body style="background:#123456"></body>

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2368: if
(ui::GrabViewSnapshot(GetView()->GetNativeView(), &data,
There is nothing wrong with your assumption, but capturing multiple times in a
loop, while knowing that the result will be the same isn't pretty. MacOS is the
only place you need to work around. I was hoping you could do something like
NSImageToSkBitmapWithColorSpace and operate on SkBitmap from there.

[Eric Seckler, 2017-01-05 16:17:07]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 16:17:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 16:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 16:32:36]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Eric Seckler, 2017-01-06 15:50:16]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[Eric Seckler, 2017-01-06 15:50:22]

Patchset #4 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-01-06 15:50:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 16:05:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 16:05:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-01-06 18:21:04]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 18:21:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 18:30:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 18:30:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Eric Seckler, 2017-01-06 19:01:28]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 19:02:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 20:05:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 20:05:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eric Seckler, 2017-01-06 20:54:54]

Patchset #4 (id:160001) has been deleted

[Eric Seckler, 2017-01-06 20:55:04]

Patchset #4 (id:180001) has been deleted

[Eric Seckler, 2017-01-06 20:57:07]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 20:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-06 21:06:01]

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:665:
content::ExecuteScript(shell()->web_contents()->GetRenderViewHost(),
On 2017/01/04 00:10:50, pfeldman wrote:
> Oh, you are using the harness one. I looked at the RVH fetch and assumed it
was
> api call. You could still use following URL to avoid roundtrip:
> 
> data:text/html,<body style="background:#123456"></body>

Done :)

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2592983002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2368: if
(ui::GrabViewSnapshot(GetView()->GetNativeView(), &data,
On 2017/01/04 00:10:50, pfeldman wrote:
> There is nothing wrong with your assumption, but capturing multiple times in a
> loop, while knowing that the result will be the same isn't pretty. MacOS is
the
> only place you need to work around. I was hoping you could do something like
> NSImageToSkBitmapWithColorSpace and operate on SkBitmap from there.

I've changed ui::Grab*Snapshot* methods to return gfx::Images that are either
backed by an NSImage (mac) or an SkBitmap (other platforms), and am converting
from either of these two representations in PageHandler, using existing /
extended helper methods. Does this make more sense?

[Eric Seckler, 2017-01-06 21:09:09]

Patchset #4 (id:200001) has been deleted

[Eric Seckler, 2017-01-06 21:09:48]

Patchset #4 (id:220001) has been deleted

[Eric Seckler, 2017-01-06 21:10:02]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 21:10:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 21:41:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:41:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pfeldman, 2017-01-07 01:13:14]

lgtm, thanks! Please run ui/macos-specific code against corresponding owners.

[pfeldman, 2017-01-07 01:13:28]

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:572:
base::PostTaskWithTraitsAndReplyWithResult(
Why is this necessary? Weren't we doing it synchronously? It might be important
for screencast to encode async, but with capture screenshot it is less important
whether you do it in this task or another.

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler_image_utils.cc (right):

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler_image_utils.cc:11:
scoped_refptr<base::RefCountedMemory> EncodeScreenshotImageAsJpeg(
Is this dead code?

[Eric Seckler, 2017-01-09 13:42:12]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 13:42:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-09 14:48:26]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch adds encoding options to
ui::Grab*Snapshot* methods.

BUG=676282
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot* methods to
return a gfx::Image instead of a png. Utility methods can then encode the
gfx::Image to jpeg/png depending on its platform-specific backing
representation.

BUG=676282
==========

[Eric Seckler, 2017-01-09 14:48:45]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot* methods to
return a gfx::Image instead of a png. Utility methods can then encode the
gfx::Image to jpeg/png depending on its platform-specific backing
representation.

BUG=676282
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282
==========

[Eric Seckler, 2017-01-09 14:52:03]

eseckler@chromium.org changed reviewers:
+ avi@chromium.org, dtrainor@chromium.org, enne@chromium.org,
oshima@chromium.org, sadrul@chromium.org
- dgozman@chromium.org

[Eric Seckler, 2017-01-09 14:52:05]

enne@, you might be able to help with a comment in
ui/snapshot/snapshot_aura_unittest.cc below.

Also adding owners:
  sadrul@ for ui/
  avi@ for *.mm in ui/
  dtrainor@ for chrome/browser/android/feedback
  oshima@ for chrome/browser/chromeos

Thank you!

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:572:
base::PostTaskWithTraitsAndReplyWithResult(
On 2017/01/07 01:13:27, pfeldman wrote:
> Why is this necessary? Weren't we doing it synchronously? It might be
important
> for screencast to encode async, but with capture screenshot it is less
important
> whether you do it in this task or another.

Great, making it synchronous :) Previously, it was half-half (png encoding
wasn't).

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler_image_utils.cc (right):

https://codereview.chromium.org/2592983002/diff/240001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler_image_utils.cc:11:
scoped_refptr<base::RefCountedMemory> EncodeScreenshotImageAsJpeg(
On 2017/01/07 01:13:27, pfeldman wrote:
> Is this dead code?

Yeah, I thought I had deleted these files.. Gone now. Thanks!

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
actually wait until the copy response is
enne@/sadrul@: These tests are super flaky and don't actually pass even before
my change. It seems they may simply not be run on the bots?

The reason for their flakiness seems to be that the WaitForDraw() below doesn't
actually ensure that the copy response was received. It looks like the
cc::Display actually calls the draw callbacks before swapping buffers. Even if
that were the case, I'm not entirely sure that the copy response from the
GLRenderer would be sent immediately during SwapBuffers (it looks async in
GLRenderer::GetFramebufferPixelsAsync).

Do you know of a more convenient event we could wait for here, or is the better
option to wait until our own callback (SnapshotHolder::SnapshotCallback) has
been run and fail through timeout otherwise?

[commit-bot: I haz the power, 2017-01-09 15:52:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 15:52:54]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-01-09 16:57:34]

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:506:
static_cast<int32_t>(expected_color)) > error_limit) {
An SkColor is a set of four 8-bit values packed into a 32-bit integer. See
SkColorSetARGB():

(alpha) | (red) | (green) | (blue)

Using std::abs in this way allows for wiggle room in the blue channel (as that
channel is in the lowest bit positions), but in no other channel. I don't see
how this is a meaningful comparison.

You have to at least compare all the channels, so if you have to write a
comparison helper function we might as well go all the way.

bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int32_t error_limit)
{
  auto r_distance = std::abs(static_cast<int32_t>(SkColorGetR(color1)) -
      static_cast<int32_t>(SkColorGetR(color2)));
  auto g_distance = std::abs(static_cast<int32_t>(SkColorGetG(color1)) -
      static_cast<int32_t>(SkColorGetG(color2)));
  auto b_distance = std::abs(static_cast<int32_t>(SkColorGetB(color1)) -
      static_cast<int32_t>(SkColorGetB(color2)));

  return r_distance * r_distance + g_distance * g_distance + b_distance *
b_distance <
      error_limit * error_limit;
}

And then use that.

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:657: // See
crbug.com/653637.
This bug is about mismatches in color. Do we want to re-enable the tests, but
for low-end devices crank up the error limit now that we have that ability? At
least we can get some basic coverage, and stop things from getting worse :)

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:210: // by an NSImage on
MacOS or by an SkBitmap otherwise. The gfx::Image may be
Why are we doing things differently on the Mac than otherwise?

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_util.h
File ui/gfx/image/image_util.h (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util.h:36: bool JPEG1xEncodedDataFromSkiaRepresentation(const
Image& image,
GFX_EXPORT?

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
File ui/gfx/image/image_util_mac.mm (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util_mac.mm:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
It's 2017.

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util_mac.mm:30: forKey:NSImageCompressionFactor];
NSDictionary* options = @{ NSImageCompressionFactor : @(compressionFactor)};

should work.

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.cc
File ui/snapshot/snapshot.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.c...
ui/snapshot/snapshot.cc:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
It's 2017.

[David Trainor- moved to gerrit, 2017-01-09 22:17:33]

chrome/browser/android/feedback lgtm!

[Avi (use Gerrit), 2017-01-09 22:59:57]

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_util.h
File ui/gfx/image/image_util.h (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util.h:36: bool JPEG1xEncodedDataFromSkiaRepresentation(const
Image& image,
On 2017/01/09 16:57:33, Avi wrote:
> GFX_EXPORT?

Ah, I see, this is for internal use only. Never mind.

[sadrul, 2017-01-10 05:12:50]

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.cc
File ui/snapshot/snapshot.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.c...
ui/snapshot/snapshot.cc:37: background_task_runner, callback));
std::move(background_task_runner)

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.h...
ui/snapshot/snapshot.h:40: const gfx::Rect& snapshot_bounds);
The out-param usually is the last parameter. Since you are here, mind making
that change?

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
actually wait until the copy response is
On 2017/01/09 14:52:05, Eric Seckler wrote:
> enne@/sadrul@: These tests are super flaky and don't actually pass even before
> my change. It seems they may simply not be run on the bots?

Ugh, it does look like these tests are not actually run on any bots :-/

> 
> The reason for their flakiness seems to be that the WaitForDraw() below
doesn't
> actually ensure that the copy response was received. It looks like the
> cc::Display actually calls the draw callbacks before swapping buffers. Even if
> that were the case, I'm not entirely sure that the copy response from the
> GLRenderer would be sent immediately during SwapBuffers (it looks async in
> GLRenderer::GetFramebufferPixelsAsync).
> 
> Do you know of a more convenient event we could wait for here, or is the
better
> option to wait until our own callback (SnapshotHolder::SnapshotCallback) has
> been run and fail through timeout otherwise?

I cannot tell you of what the best fix would be, but the option you suggest
doesn't sound like a bad one. enne@ can tell you what the right fix should be.

[enne (OOO), 2017-01-10 19:26:04]

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
actually wait until the copy response is
On 2017/01/10 at 05:12:50, sadrul wrote:
> On 2017/01/09 14:52:05, Eric Seckler wrote:
> > enne@/sadrul@: These tests are super flaky and don't actually pass even
before
> > my change. It seems they may simply not be run on the bots?
> 
> Ugh, it does look like these tests are not actually run on any bots :-/
> 
> > 
> > The reason for their flakiness seems to be that the WaitForDraw() below
doesn't
> > actually ensure that the copy response was received. It looks like the
> > cc::Display actually calls the draw callbacks before swapping buffers. Even
if
> > that were the case, I'm not entirely sure that the copy response from the
> > GLRenderer would be sent immediately during SwapBuffers (it looks async in
> > GLRenderer::GetFramebufferPixelsAsync).
> > 
> > Do you know of a more convenient event we could wait for here, or is the
better
> > option to wait until our own callback (SnapshotHolder::SnapshotCallback) has
> > been run and fail through timeout otherwise?
> 
> I cannot tell you of what the best fix would be, but the option you suggest
doesn't sound like a bad one. enne@ can tell you what the right fix should be.

It sounds likely that this has been broken ever since there was a cc::Display. 
I think waiting on your own callback is going to be the most robust solution
into the future.

[Eric Seckler, 2017-01-11 13:49:07]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 13:49:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 14:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 14:02:13]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Eric Seckler, 2017-01-11 14:04:28]

Patchset #6 (id:280001) has been deleted

[Eric Seckler, 2017-01-11 14:10:56]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 14:11:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-11 14:16:40]

Patchset #6 (id:300001) has been deleted

[commit-bot: I haz the power, 2017-01-11 14:20:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2017-01-11 14:22:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 14:22:58]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Eric Seckler, 2017-01-11 14:24:23]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 14:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 15:51:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 15:51:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-01-11 15:58:44]

Thanks everyone, PTAL! :)

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:506:
static_cast<int32_t>(expected_color)) > error_limit) {
On 2017/01/09 16:57:33, Avi wrote:
> An SkColor is a set of four 8-bit values packed into a 32-bit integer. See
> SkColorSetARGB():
> 
> (alpha) | (red) | (green) | (blue)
> 
> Using std::abs in this way allows for wiggle room in the blue channel (as that
> channel is in the lowest bit positions), but in no other channel. I don't see
> how this is a meaningful comparison.
> 
> You have to at least compare all the channels, so if you have to write a
> comparison helper function we might as well go all the way.
> 
> bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int32_t
error_limit)
> {
>   auto r_distance = std::abs(static_cast<int32_t>(SkColorGetR(color1)) -
>       static_cast<int32_t>(SkColorGetR(color2)));
>   auto g_distance = std::abs(static_cast<int32_t>(SkColorGetG(color1)) -
>       static_cast<int32_t>(SkColorGetG(color2)));
>   auto b_distance = std::abs(static_cast<int32_t>(SkColorGetB(color1)) -
>       static_cast<int32_t>(SkColorGetB(color2)));
> 
>   return r_distance * r_distance + g_distance * g_distance + b_distance *
> b_distance <
>       error_limit * error_limit;
> }
> 
> And then use that.

Uhh, yeah. Wasn't thinking here. Thank you for the helper function!

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:657: // See
crbug.com/653637.
On 2017/01/09 16:57:33, Avi wrote:
> This bug is about mismatches in color. Do we want to re-enable the tests, but
> for low-end devices crank up the error limit now that we have that ability? At
> least we can get some basic coverage, and stop things from getting worse :)

I'm actually not sure if this would still be happening on the bots, since the
original bot that was flaky doesn't seem to exist anymore. Thus, I'd be happy to
re-enable and (if necessary) use a higher error limit on low-end devices - but
let's do that separately :)

Added a todo for this here.

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:210: // by an NSImage on
MacOS or by an SkBitmap otherwise. The gfx::Image may be
On 2017/01/09 16:57:33, Avi wrote:
> Why are we doing things differently on the Mac than otherwise?

Everywhere but on Mac, we're grabbing from cc layers (i.e. receive SkBitmaps
directly), but on Mac, we're taking the screenshot using Cocoa. The reason for
this seems to be that DevTools' Page.CaptureScreenshot is used for end-to-end
integration-testing, see earlier comments by Pavel.

To avoid unnecessary conversion from Cocoa's NSImage to an SkBitmap and then to
a jpeg/png, we're using a gfx::Image with different representations, which we
can then convert to jpeg/png directly from the respective representation.

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
File ui/gfx/image/image_util_mac.mm (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util_mac.mm:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2017/01/09 16:57:33, Avi wrote:
> It's 2017.

Done.

https://codereview.chromium.org/2592983002/diff/260001/ui/gfx/image/image_uti...
ui/gfx/image/image_util_mac.mm:30: forKey:NSImageCompressionFactor];
On 2017/01/09 16:57:33, Avi wrote:
> NSDictionary* options = @{ NSImageCompressionFactor : @(compressionFactor)};
> 
> should work.

Done.

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.cc
File ui/snapshot/snapshot.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.c...
ui/snapshot/snapshot.cc:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
On 2017/01/09 16:57:33, Avi wrote:
> It's 2017.

Done.

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.c...
ui/snapshot/snapshot.cc:37: background_task_runner, callback));
On 2017/01/10 05:12:49, sadrul wrote:
> std::move(background_task_runner)

Done.

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot.h...
ui/snapshot/snapshot.h:40: const gfx::Rect& snapshot_bounds);
On 2017/01/10 05:12:50, sadrul wrote:
> The out-param usually is the last parameter. Since you are here, mind making
> that change?

Sure :)

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
actually wait until the copy response is
On 2017/01/10 19:26:04, enne wrote:
> On 2017/01/10 at 05:12:50, sadrul wrote:
> > On 2017/01/09 14:52:05, Eric Seckler wrote:
> > > enne@/sadrul@: These tests are super flaky and don't actually pass even
> before
> > > my change. It seems they may simply not be run on the bots?
> > 
> > Ugh, it does look like these tests are not actually run on any bots :-/
> > 
> > > 
> > > The reason for their flakiness seems to be that the WaitForDraw() below
> doesn't
> > > actually ensure that the copy response was received. It looks like the
> > > cc::Display actually calls the draw callbacks before swapping buffers.
Even
> if
> > > that were the case, I'm not entirely sure that the copy response from the
> > > GLRenderer would be sent immediately during SwapBuffers (it looks async in
> > > GLRenderer::GetFramebufferPixelsAsync).
> > > 
> > > Do you know of a more convenient event we could wait for here, or is the
> better
> > > option to wait until our own callback (SnapshotHolder::SnapshotCallback)
has
> > > been run and fail through timeout otherwise?
> > 
> > I cannot tell you of what the best fix would be, but the option you suggest
> doesn't sound like a bad one. enne@ can tell you what the right fix should be.
> 
> It sounds likely that this has been broken ever since there was a cc::Display.

> I think waiting on your own callback is going to be the most robust solution
> into the future.

OK, thanks!

Turns out, there's more failing than just this (probably for a while as well) -
apparently the screenshot bounds are affected by test_screen()->SetUIScale(),
even though the tests don't expect them to be:

[ RUN      ] SnapshotAuraTest.UIScale
../../ui/snapshot/snapshot_aura_unittest.cc:234: Failure
Value of: snapshot.Size().ToString()
  Actual: "240x160"
Expected: gfx::ToRoundedSize(snapshot_size).ToString()
Which is: "300x200"

Can you tell whether this is a valid bug - or incorrect tests?

[Avi (use Gerrit), 2017-01-11 17:09:20]

lgtm

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:657: // See
crbug.com/653637.
On 2017/01/11 15:58:44, Eric Seckler wrote:
> On 2017/01/09 16:57:33, Avi wrote:
> > This bug is about mismatches in color. Do we want to re-enable the tests,
but
> > for low-end devices crank up the error limit now that we have that ability?
At
> > least we can get some basic coverage, and stop things from getting worse :)
> 
> I'm actually not sure if this would still be happening on the bots, since the
> original bot that was flaky doesn't seem to exist anymore. Thus, I'd be happy
to
> re-enable and (if necessary) use a higher error limit on low-end devices - but
> let's do that separately :)
> 
> Added a todo for this here.

SGTM!

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:210: // by an NSImage on
MacOS or by an SkBitmap otherwise. The gfx::Image may be
On 2017/01/11 15:58:44, Eric Seckler wrote:
> On 2017/01/09 16:57:33, Avi wrote:
> > Why are we doing things differently on the Mac than otherwise?
> 
> Everywhere but on Mac, we're grabbing from cc layers (i.e. receive SkBitmaps
> directly), but on Mac, we're taking the screenshot using Cocoa. The reason for
> this seems to be that DevTools' Page.CaptureScreenshot is used for end-to-end
> integration-testing, see earlier comments by Pavel.
> 
> To avoid unnecessary conversion from Cocoa's NSImage to an SkBitmap and then
to
> a jpeg/png, we're using a gfx::Image with different representations, which we
> can then convert to jpeg/png directly from the respective representation.

Can we have some kind of note in the code? It's also weird that we're doing
different integration testing on different platforms forcing us to have
different app code.

[sadrul, 2017-01-11 17:16:54]

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
actually wait until the copy response is
On 2017/01/11 15:58:44, Eric Seckler wrote:
> On 2017/01/10 19:26:04, enne wrote:
> > On 2017/01/10 at 05:12:50, sadrul wrote:
> > > On 2017/01/09 14:52:05, Eric Seckler wrote:
> > > > enne@/sadrul@: These tests are super flaky and don't actually pass even
> > before
> > > > my change. It seems they may simply not be run on the bots?
> > > 
> > > Ugh, it does look like these tests are not actually run on any bots :-/
> > > 
> > > > 
> > > > The reason for their flakiness seems to be that the WaitForDraw() below
> > doesn't
> > > > actually ensure that the copy response was received. It looks like the
> > > > cc::Display actually calls the draw callbacks before swapping buffers.
> Even
> > if
> > > > that were the case, I'm not entirely sure that the copy response from
the
> > > > GLRenderer would be sent immediately during SwapBuffers (it looks async
in
> > > > GLRenderer::GetFramebufferPixelsAsync).
> > > > 
> > > > Do you know of a more convenient event we could wait for here, or is the
> > better
> > > > option to wait until our own callback (SnapshotHolder::SnapshotCallback)
> has
> > > > been run and fail through timeout otherwise?
> > > 
> > > I cannot tell you of what the best fix would be, but the option you
suggest
> > doesn't sound like a bad one. enne@ can tell you what the right fix should
be.
> > 
> > It sounds likely that this has been broken ever since there was a
cc::Display.
> 
> > I think waiting on your own callback is going to be the most robust solution
> > into the future.
> 
> OK, thanks!
> 
> Turns out, there's more failing than just this (probably for a while as well)
-
> apparently the screenshot bounds are affected by test_screen()->SetUIScale(),
> even though the tests don't expect them to be:
> 
> [ RUN      ] SnapshotAuraTest.UIScale
> ../../ui/snapshot/snapshot_aura_unittest.cc:234: Failure
> Value of: snapshot.Size().ToString()
>   Actual: "240x160"
> Expected: gfx::ToRoundedSize(snapshot_size).ToString()
> Which is: "300x200"
> 
> Can you tell whether this is a valid bug - or incorrect tests?

I think the current expectations are correct. Sounds like a valid bug? oshima@
can confirm.

[oshima, 2017-01-13 22:08:27]

On 2017/01/11 17:16:54, sadrul wrote:
>
https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
> File ui/snapshot/snapshot_aura_unittest.cc (right):
> 
>
https://codereview.chromium.org/2592983002/diff/260001/ui/snapshot/snapshot_a...
> ui/snapshot/snapshot_aura_unittest.cc:150: // TODO(eseckler): This doesn't
> actually wait until the copy response is
> On 2017/01/11 15:58:44, Eric Seckler wrote:
> > On 2017/01/10 19:26:04, enne wrote:
> > > On 2017/01/10 at 05:12:50, sadrul wrote:
> > > > On 2017/01/09 14:52:05, Eric Seckler wrote:
> > > > > enne@/sadrul@: These tests are super flaky and don't actually pass
even
> > > before
> > > > > my change. It seems they may simply not be run on the bots?
> > > > 
> > > > Ugh, it does look like these tests are not actually run on any bots :-/
> > > > 
> > > > > 
> > > > > The reason for their flakiness seems to be that the WaitForDraw()
below
> > > doesn't
> > > > > actually ensure that the copy response was received. It looks like the
> > > > > cc::Display actually calls the draw callbacks before swapping buffers.
> > Even
> > > if
> > > > > that were the case, I'm not entirely sure that the copy response from
> the
> > > > > GLRenderer would be sent immediately during SwapBuffers (it looks
async
> in
> > > > > GLRenderer::GetFramebufferPixelsAsync).
> > > > > 
> > > > > Do you know of a more convenient event we could wait for here, or is
the
> > > better
> > > > > option to wait until our own callback
(SnapshotHolder::SnapshotCallback)
> > has
> > > > > been run and fail through timeout otherwise?
> > > > 
> > > > I cannot tell you of what the best fix would be, but the option you
> suggest
> > > doesn't sound like a bad one. enne@ can tell you what the right fix should
> be.
> > > 
> > > It sounds likely that this has been broken ever since there was a
> cc::Display.
> > 
> > > I think waiting on your own callback is going to be the most robust
solution
> > > into the future.
> > 
> > OK, thanks!
> > 
> > Turns out, there's more failing than just this (probably for a while as
well)
> -
> > apparently the screenshot bounds are affected by
test_screen()->SetUIScale(),
> > even though the tests don't expect them to be:
> > 
> > [ RUN      ] SnapshotAuraTest.UIScale
> > ../../ui/snapshot/snapshot_aura_unittest.cc:234: Failure
> > Value of: snapshot.Size().ToString()
> >   Actual: "240x160"
> > Expected: gfx::ToRoundedSize(snapshot_size).ToString()
> > Which is: "300x200"
> > 
> > Can you tell whether this is a valid bug - or incorrect tests?
> 
> I think the current expectations are correct. Sounds like a valid bug? oshima@
> can confirm.

I believe 240x160 is correct, and the current expectation is wrong.
The image should be recorded in the pixels not scaled size.

[Eric Seckler, 2017-01-16 10:41:00]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 10:41:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 10:51:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 10:51:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-01-16 15:17:32]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 15:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-16 15:28:37]

Patchset #7 (id:340001) has been deleted

[Eric Seckler, 2017-01-16 15:28:52]

Patchset #7 (id:360001) has been deleted

[Eric Seckler, 2017-01-16 15:29:04]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 15:29:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 17:36:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:36:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-01-19 09:42:21]

Patchset #8 (id:400001) has been deleted

[Eric Seckler, 2017-01-19 12:36:23]

Patchset #9 (id:440001) has been deleted

[Eric Seckler, 2017-01-19 13:36:36]

Patchset #7 (id:380001) has been deleted

[Eric Seckler, 2017-01-19 13:36:46]

Patchset #7 (id:420001) has been deleted

[Eric Seckler, 2017-01-19 13:37:00]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 13:37:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-19 13:49:11]

eseckler@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Eric Seckler, 2017-01-19 13:49:12]

Sadrul, PTAL.

oshima@, can you also have a look at the chromeos bits, please?

+phajdan.jr@ for testing/buildbot.

Thanks everyone!

On 2017/01/13 22:08:27, oshima wrote:
> On 2017/01/11 17:16:54, sadrul wrote:
> > I think the current expectations are correct. Sounds like a valid bug?
oshima@
> > can confirm.
> 
> I believe 240x160 is correct, and the current expectation is wrong.
> The image should be recorded in the pixels not scaled size.

OK, I've updated the expectations. Tests are now passing - the mac tests also
needed a few changes.

I'm also enabling them on the trybots (mac and linux), since it sounds like it
was an oversight that they aren't currently run at all.

[commit-bot: I haz the power, 2017-01-19 15:33:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:33:06]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-01-20 00:34:46]

c/b/chromeos lgtm

[Eric Seckler, 2017-01-20 09:22:12]

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2592983002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:210: // by an NSImage on
MacOS or by an SkBitmap otherwise. The gfx::Image may be
On 2017/01/11 17:09:20, Avi (offsites OOO 17Jan-2Feb) wrote:
> On 2017/01/11 15:58:44, Eric Seckler wrote:
> > On 2017/01/09 16:57:33, Avi wrote:
> > > Why are we doing things differently on the Mac than otherwise?
> > 
> > Everywhere but on Mac, we're grabbing from cc layers (i.e. receive SkBitmaps
> > directly), but on Mac, we're taking the screenshot using Cocoa. The reason
for
> > this seems to be that DevTools' Page.CaptureScreenshot is used for
end-to-end
> > integration-testing, see earlier comments by Pavel.
> > 
> > To avoid unnecessary conversion from Cocoa's NSImage to an SkBitmap and then
> to
> > a jpeg/png, we're using a gfx::Image with different representations, which
we
> > can then convert to jpeg/png directly from the respective representation.
> 
> Can we have some kind of note in the code? It's also weird that we're doing
> different integration testing on different platforms forcing us to have
> different app code.

Added a comment here. As I understand it, Mac is sort of a special case, because
some Cocoa features actually affect how things look, whereas on the other OSes,
aura handles pretty much everything (and thus, it suffices to compare
screenshots of aura/compositor layers there).

[sadrul, 2017-01-20 16:15:19]

lgtm

[Eric Seckler, 2017-01-20 16:29:21]

eseckler@chromium.org changed reviewers:
+ dpranke@chromium.org
- phajdan.jr@chromium.org

[Eric Seckler, 2017-01-20 16:29:23]

-Pawel (ooo)
+dpranke@ instead (for testing/buildbot).

Dirk, please let me know if you have any questions/objections to enabling these
tests on the bots. Thanks!

[Dirk Pranke, 2017-01-23 23:27:55]

lgtm

[Eric Seckler, 2017-01-24 11:01:42]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-01-24 11:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, dtrainor@chromium.org, avi@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2592983002/#ps480001
(title: "rebase & added a comment.")

[commit-bot: I haz the power, 2017-01-24 11:02:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 12:15:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 12:15:18]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-01-24 12:25:46]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 12:25:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 14:02:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 14:02:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-01-24 14:20:50]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 14:21:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-24 15:14:23]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2017-01-24 15:14:23]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2017-01-24 15:14:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 15:18:59]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1485270863934020,
"parent_rev": "0cfc6e8dba6c89bb403259df01135775199c4309", "commit_rev":
"b6438b1ca6462f3cc014cd1bf18410957101a119"}

[commit-bot: I haz the power, 2017-01-24 15:19:33]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282

Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#445730}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...
==========

[commit-bot: I haz the power, 2017-01-24 15:19:35]

Committed patchset #8 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...

[mohsen, 2017-01-24 16:31:44]

A revert of this CL (patchset #8 id:480001) has been created in
https://codereview.chromium.org/2650903003/ by mohsen@chromium.org.

The reason for reverting is: Causing failures on "Mac10.11 Tests" builder:

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.11%20Tests...

[ RUN      ] GrabWindowSnapshotTest.TestGrabWindowSnapshot
../../ui/snapshot/snapshot_mac_unittest.mm:45: Failure
Value of: CGImageGetWidth([rep CGImage])
  Actual: 1
Expected: 400 * scaleFactor
Which is: 400
../../ui/snapshot/snapshot_mac_unittest.mm:49: Failure
Expected: (red + green + blue) >= (3.0), actual: 0 vs 3
[  FAILED  ] GrabWindowSnapshotTest.TestGrabWindowSnapshot (31 ms)
.

[findit-for-me, 2017-01-24 16:31:45]

FYI: Findit identified this CL at revision 445730 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Eric Seckler, 2017-01-24 18:56:08]

I could reproduce this on a 10.12 macbook (so looks like it's broken >= macos
10.11) and traced this down to CGWindowListCreateImage in snapshot_mac.cc, which
returns a 1x1 pixel image.

But I don't know what is causing this. Maybe the window is set up wrongly in the
test?

Anyway, the problem is not something introduced in this patch - it exists in the
original snapshot_mac_unittest.cc as well.
If nobody has an idea of what may be causing it, I'm leaning towards disabling
the test on the Mac 10.11 bots and filing a bug for now.

Btw, screenshots in general seem to work on the 10.11 builder, since the
CaptureScreenshotTest in content_browsertests ran successfully.

On 2017/01/24 16:31:44, mohsen wrote:
> A revert of this CL (patchset #8 id:480001) has been created in
> https://codereview.chromium.org/2650903003/ by mailto:mohsen@chromium.org.
> 
> The reason for reverting is: Causing failures on "Mac10.11 Tests" builder:
> 
>
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.11%20Tests...
> 
> [ RUN      ] GrabWindowSnapshotTest.TestGrabWindowSnapshot
> ../../ui/snapshot/snapshot_mac_unittest.mm:45: Failure
> Value of: CGImageGetWidth([rep CGImage])
>   Actual: 1
> Expected: 400 * scaleFactor
> Which is: 400
> ../../ui/snapshot/snapshot_mac_unittest.mm:49: Failure
> Expected: (red + green + blue) >= (3.0), actual: 0 vs 3
> [  FAILED  ] GrabWindowSnapshotTest.TestGrabWindowSnapshot (31 ms)
> .

[Eric Seckler, 2017-01-25 09:39:42]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282

Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#445730}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282

Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#445730}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...
==========

[Eric Seckler, 2017-01-25 09:41:26]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 09:41:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-25 10:46:43]

On 2017/01/24 18:56:08, Eric Seckler wrote:
> I could reproduce this on a 10.12 macbook (so looks like it's broken >= macos
> 10.11) and traced this down to CGWindowListCreateImage in snapshot_mac.cc,
which
> returns a 1x1 pixel image.
> 
> But I don't know what is causing this. Maybe the window is set up wrongly in
the
> test?
> 
> Anyway, the problem is not something introduced in this patch - it exists in
the
> original snapshot_mac_unittest.cc as well.
> If nobody has an idea of what may be causing it, I'm leaning towards disabling
> the test on the Mac 10.11 bots and filing a bug for now.
> 
> Btw, screenshots in general seem to work on the 10.11 builder, since the
> CaptureScreenshotTest in content_browsertests ran successfully.

Test should now be skipped for >=10.11, filed https://crbug.com/685088.
I'll try to reland.

[commit-bot: I haz the power, 2017-01-25 11:46:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 11:46:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eric Seckler, 2017-01-25 11:50:17]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 11:50:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 14:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 14:48:30]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-01-25 15:02:13]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-01-25 15:02:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, avi@chromium.org, sadrul@chromium.org,
oshima@chromium.org, dtrainor@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2592983002/#ps500001
(title: "Disabled test on MacOS >=10.11, added ref to bug.")

[commit-bot: I haz the power, 2017-01-25 15:02:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 15:07:58]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1485356533172010,
"parent_rev": "a027199ed55462ac711f04cc5c23eff59678beb1", "commit_rev":
"7233c1a70ea0194caf10f4365cf3520e5be7aa8f"}

[commit-bot: I haz the power, 2017-01-25 15:08:39]

Description was changed from

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282

Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#445730}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...
==========

to

==========
[devtools] Support different encodings for Page.CaptureScreenshot.

This adds support for jpeg encoding of screenshots, similar to that for
screencasts. For this purpose, the patch modifies ui::Grab*Snapshot*
methods to return a gfx::Image instead of a png. Utility methods can
then encode the gfx::Image to jpeg/png depending on its
platform-specific backing representation.

BUG=676282

Review-Url: https://codereview.chromium.org/2592983002
Cr-Original-Commit-Position: refs/heads/master@{#445730}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6438b1ca6462f3cc014cd1bf184...
Review-Url: https://codereview.chromium.org/2592983002
Cr-Commit-Position: refs/heads/master@{#446021}
Committed:
https://chromium.googlesource.com/chromium/src/+/7233c1a70ea0194caf10f4365cf3...
==========

[commit-bot: I haz the power, 2017-01-25 15:08:41]

Committed patchset #9 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/7233c1a70ea0194caf10f4365cf3...