Update device permissions dialog ui for Chrome apps and extensions

Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
Committed: https://crrev.com/43003629cf2cca816884c226f18d32bc96236bb5
Cr-Commit-Position: refs/heads/master@{#435075}

[juncai, 2016-10-05 01:20:01]

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

[commit-bot: I haz the power, 2016-10-05 01:20:38]

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-10-05 02:33:18]

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

[commit-bot: I haz the power, 2016-10-05 02:33:19]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-11-22 01:49:57]

Description was changed from

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL updated device permissions dialog by reusing the chooser ui
that is used by WebUSB and WebBluetooth.

BUG=652445
==========

to

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL updated device permissions dialog by reusing the chooser ui
that is used by WebUSB and WebBluetooth.

BUG=652445, 653228
==========

[juncai, 2016-11-22 02:03:03]

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

[commit-bot: I haz the power, 2016-11-22 02:03:49]

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-11-22 03:07:24]

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

[commit-bot: I haz the power, 2016-11-22 03:07:25]

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_...)

[juncai, 2016-11-22 21:21:12]

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

[commit-bot: I haz the power, 2016-11-22 21:21:37]

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

[juncai, 2016-11-22 23:46:00]

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

[commit-bot: I haz the power, 2016-11-22 23:46:34]

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

[juncai, 2016-11-22 23:50:10]

Description was changed from

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL updated device permissions dialog by reusing the chooser ui
that is used by WebUSB and WebBluetooth.

BUG=652445, 653228
==========

to

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL updated device permissions dialog by reusing the chooser ui
that is used by WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

[juncai, 2016-11-23 00:35:07]

Description was changed from

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL updated device permissions dialog by reusing the chooser ui
that is used by WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

to

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

[juncai, 2016-11-23 00:36:05]

juncai@chromium.org changed reviewers:
+ reillyg@chromium.org

[juncai, 2016-11-23 00:36:06]

Please take a look.

[commit-bot: I haz the power, 2016-11-23 01:37:04]

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

[commit-bot: I haz the power, 2016-11-23 01:37:05]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-11-23 02:07:48]

As discussed offline we can get rid of the dedicated serial number column but
need the same logic we have in WebUSB to display it if we have two devices with
otherwise identical names.

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:261: }
nit: while you're modifying this statement, remove the braces

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:336: }
nit: while you're modifying this statement, remove the braces

[juncai, 2016-11-23 20:19:32]

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

[commit-bot: I haz the power, 2016-11-23 20:20:16]

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-11-23 20:24:05]

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

[commit-bot: I haz the power, 2016-11-23 20:24:06]

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

[juncai, 2016-11-23 20:38:53]

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

[commit-bot: I haz the power, 2016-11-23 20:39:35]

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

[juncai, 2016-11-23 22:36:57]

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

[commit-bot: I haz the power, 2016-11-23 22:37:51]

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

[juncai, 2016-11-23 22:43:48]

Added code to also display device serial number when devices have the same name.

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:261: }
On 2016/11/23 02:07:48, Reilly Grant wrote:
> nit: while you're modifying this statement, remove the braces

Done.

https://codereview.chromium.org/2390823005/diff/60001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:336: }
On 2016/11/23 02:07:48, Reilly Grant wrote:
> nit: while you're modifying this statement, remove the braces

Done.

[commit-bot: I haz the power, 2016-11-24 00:40:43]

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

[commit-bot: I haz the power, 2016-11-24 00:40:44]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[juncai, 2016-11-28 18:47:49]

juncai@chromium.org changed reviewers:
+ rsesek@chromium.org, sky@chromium.org

[juncai, 2016-11-28 18:47:50]

sky@chromium.org: Please review changes in 
//chrome/browser/ui/views

rsesek@chromium.org: Please review changes in 
//chrome/app/nibs
//chrome/browser/ui/cocoa

[Reilly Grant (use Gerrit), 2016-11-28 20:04:28]

lgtm

[Robert Sesek, 2016-11-28 22:20:49]

LGTM w/ a question

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:455: helpButton_ =
Should you create the helpButton_ if !ShouldShowFootnoteView()?

[sky, 2016-11-28 23:13:21]

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:288: if
(chooser_controller_->ShouldShowFootnoteView())
Can ShouldShowFootnoteView change value? Assuming it can't did you consider only
creating footnote_link_ when necessary? It would mean more null checks, but that
way it forces people to think about what they use the footnote_link_ for as it
might be null.

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:145:
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents());
How is it possible to end up here with no WebContentsModalDialogManager? If you
call Dismissed, why do you need to create ChooserController? And how do you know
the delay is the right thing?

[juncai, 2016-11-29 18:04:34]

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

[commit-bot: I haz the power, 2016-11-29 18:04:57]

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

[juncai, 2016-11-29 18:10:08]

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:455: helpButton_ =
On 2016/11/28 22:20:49, Robert Sesek wrote:
> Should you create the helpButton_ if !ShouldShowFootnoteView()?

The |helpButton_| doesn't need to be created if !ShouldShowFootnoteView().
Updated code to address it.

Done.

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:288: if
(chooser_controller_->ShouldShowFootnoteView())
On 2016/11/28 23:13:21, sky wrote:
> Can ShouldShowFootnoteView change value? Assuming it can't did you consider
only
> creating footnote_link_ when necessary? It would mean more null checks, but
that
> way it forces people to think about what they use the footnote_link_ for as it
> might be null.

Done.

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2390823005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:145:
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents());
On 2016/11/28 23:13:21, sky wrote:
> How is it possible to end up here with no WebContentsModalDialogManager? If
you
> call Dismissed, why do you need to create ChooserController? And how do you
know
> the delay is the right thing?

Done.

[commit-bot: I haz the power, 2016-11-29 19:04:56]

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

[commit-bot: I haz the power, 2016-11-29 19:04:56]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-29 20:13:17]

LGTM

[juncai, 2016-11-29 20:33:06]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-11-29 20:33:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2390823005/#ps140001
(title: "address more comments")

[commit-bot: I haz the power, 2016-11-29 20:33: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, 2016-11-29 20:43:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480451586116180,
"parent_rev": "551c5c08abff1c647a9db8c026982c05cc32f37f", "commit_rev":
"06718fab5d627129c08be5b3247e701fcf3923ef"}

[commit-bot: I haz the power, 2016-11-29 20:43:54]

Description was changed from

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

to

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

[commit-bot: I haz the power, 2016-11-29 20:43:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-29 20:48:29]

Description was changed from

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228
==========

to

==========
Update device permissions dialog ui for Chrome apps and extensions

This CL is part 2 of changing Chrome Apps device permissions dialog
using the same chooser dialog as WebUSB and WebBluetooth.

I uploaded some screenshots on the issue page.

BUG=652445, 653228

Committed: https://crrev.com/43003629cf2cca816884c226f18d32bc96236bb5
Cr-Commit-Position: refs/heads/master@{#435075}
==========

[commit-bot: I haz the power, 2016-11-29 20:48:30]

Patchset 8 (id:??) landed as
https://crrev.com/43003629cf2cca816884c226f18d32bc96236bb5
Cr-Commit-Position: refs/heads/master@{#435075}