Devtools: Add screenshot button to device mode toolbar.

Devtools: Add screenshot button to device mode toolbar.

(taking over samli's patch).

Committed: https://crrev.com/0348594711e4cb5bec1fb5aa88c00266580ce259
Cr-Commit-Position: refs/heads/master@{#383796}

[pfeldman, 2016-03-25 22:53:12]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org, samli@chromium.org

[dgozman, 2016-03-28 16:33:20]

lgtm

[pfeldman, 2016-03-28 17:36:09]

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

[pfeldman, 2016-03-28 17:36:09]

+asanka@ for download OWNERS review.

[asanka, 2016-03-28 18:28:37]

On 2016/03/28 at 17:36:09, pfeldman wrote:
> +asanka@ for download OWNERS review.

lgtm.

How hard would it be to write a test (or adjust an existing test) to exercise
the download_io_controller changes? You'd need to call
BrowserWindow::IsDownloadShelfVisible() after initiating a download from a
devtools window. Only applicable for OS_WIN, OS_MAC, OS_LINUX, and (OS_CHROMEOS
&& !DownloadNotificationManager::IsEnabled()). In other cases we don't use a
browser window specific download shelf.

[pfeldman, 2016-03-28 19:02:54]

> How hard would it be to write a test (or adjust an existing test) to exercise
> the download_io_controller changes? You'd need to call
> BrowserWindow::IsDownloadShelfVisible() after initiating a download from a
> devtools window. Only applicable for OS_WIN, OS_MAC, OS_LINUX, and
(OS_CHROMEOS
> && !DownloadNotificationManager::IsEnabled()). In other cases we don't use a
> browser window specific download shelf.

In order to provide a nice test, I would need to move this code from delegate
(DownloadShelfUIControllerDelegate::OnNewDownloadReady) into the controller
(DownloadUIController::OnDownloadUpdated). I would do that, but it makes the
OnNewDownloadReady signature ugly - it changes from 

DownloadUIController::Delegate::OnNewDownloadReady(content::DownloadItem* item)

to

DownloadUIController::Delegate::OnNewDownloadReady(content::WebContents*,
content::DownloadItem* item)

which might confuse client.

Wdyt?

[asanka, 2016-03-28 19:25:03]

On 2016/03/28 at 19:02:54, pfeldman wrote:
> > How hard would it be to write a test (or adjust an existing test) to
exercise
> > the download_io_controller changes? You'd need to call
> > BrowserWindow::IsDownloadShelfVisible() after initiating a download from a
> > devtools window. Only applicable for OS_WIN, OS_MAC, OS_LINUX, and
(OS_CHROMEOS
> > && !DownloadNotificationManager::IsEnabled()). In other cases we don't use a
> > browser window specific download shelf.
> 
> In order to provide a nice test, I would need to move this code from delegate
(DownloadShelfUIControllerDelegate::OnNewDownloadReady) into the controller
(DownloadUIController::OnDownloadUpdated). I would do that, but it makes the
OnNewDownloadReady signature ugly - it changes from 
> 
> DownloadUIController::Delegate::OnNewDownloadReady(content::DownloadItem*
item)
> 
> to
> 
> DownloadUIController::Delegate::OnNewDownloadReady(content::WebContents*,
content::DownloadItem* item)
> 
> which might confuse client.
> 
> Wdyt?

We are choosing the "effective" WebContents so that the download shelf appears
on the correct browser window, right? That line of reasoning is download shelf
specific. So I'd like to keep the logic in DownloadShelfUIControllerDelegate.

Would it be too heavyweight to use a browser test for this? I.e. have a browser
test open Devtools in a separate browser window and initiate a download, then
see which browser window shows a download shelf.

[pfeldman, 2016-03-28 20:27:19]

> Would it be too heavyweight to use a browser test for this? I.e. have a
browser
> test open Devtools in a separate browser window and initiate a download, then
> see which browser window shows a download shelf.

I can try that, but it'll be a devtools sanity browsertest.

[asanka, 2016-03-28 20:28:25]

On 2016/03/28 at 20:27:19, pfeldman wrote:
> > Would it be too heavyweight to use a browser test for this? I.e. have a
browser
> > test open Devtools in a separate browser window and initiate a download,
then
> > see which browser window shows a download shelf.
> 
> I can try that, but it'll be a devtools sanity browsertest.

That should be fine as long as there's some test that will catch a regression.

[pfeldman, 2016-03-28 21:45:37]

On 2016/03/28 20:27:19, pfeldman wrote:
> > Would it be too heavyweight to use a browser test for this? I.e. have a
> browser
> > test open Devtools in a separate browser window and initiate a download,
then
> > see which browser window shows a download shelf.
> 
> I can try that, but it'll be a devtools sanity browsertest.

A simple one did not work. I am running the message loop while waiting for the
OnDownloadCreated and it does not arrive in a browser test. Mind me landing
without a test?

[pfeldman, 2016-03-29 00:40:28]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-29 00:40:28]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1840533002/#ps20001
(title: "missing namespace specified")

[commit-bot: I haz the power, 2016-03-29 00:40:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840533002/20001

[commit-bot: I haz the power, 2016-03-29 00:43:11]

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

[commit-bot: I haz the power, 2016-03-29 00:43:13]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-03-29 01:00:42]

On 2016/03/28 at 21:45:37, pfeldman wrote:
> On 2016/03/28 20:27:19, pfeldman wrote:
> > > Would it be too heavyweight to use a browser test for this? I.e. have a
> > browser
> > > test open Devtools in a separate browser window and initiate a download,
then
> > > see which browser window shows a download shelf.
> > 
> > I can try that, but it'll be a devtools sanity browsertest.
> 
> A simple one did not work. I am running the message loop while waiting for the
OnDownloadCreated and it does not arrive in a browser test. Mind me landing
without a test?

No objection.

[pfeldman, 2016-03-29 18:08:31]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-29 18:08:32]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1840533002/#ps40001
(title: "rebaselined")

[commit-bot: I haz the power, 2016-03-29 18:09:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840533002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840533002/40001

[pfeldman, 2016-03-29 18:11:03]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2016-03-29 18:12:42]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-29 18:12:43]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1840533002/#ps60001
(title: "rebaselined again")

[commit-bot: I haz the power, 2016-03-29 18:12:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840533002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840533002/60001

[commit-bot: I haz the power, 2016-03-29 19:51:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-29 19:52:46]

Description was changed from

==========
Devtools: Add screenshot button to device mode toolbar.

(taking over samli's patch).
==========

to

==========
Devtools: Add screenshot button to device mode toolbar.

(taking over samli's patch).

Committed: https://crrev.com/0348594711e4cb5bec1fb5aa88c00266580ce259
Cr-Commit-Position: refs/heads/master@{#383796}
==========

[commit-bot: I haz the power, 2016-03-29 19:52:47]

Patchset 4 (id:??) landed as
https://crrev.com/0348594711e4cb5bec1fb5aa88c00266580ce259
Cr-Commit-Position: refs/heads/master@{#383796}