[MD Extensions] Update Detail View UI

[MD Extensions] Update Detail View UI

We have new mocks!  Update the detail view UI to be more in-line with
the new specs.

Update tests accordingly.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2723613004
Cr-Commit-Position: refs/heads/master@{#455099}
Committed: https://chromium.googlesource.com/chromium/src/+/7615c37d20b7b4a57cf9b4ae7e3034e63003b2ec

[Devlin, 2017-02-28 20:12:29]

Description was changed from

==========
inprog
BUG=
==========

to

==========
inprog
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2017-02-28 23:06:49]

Description was changed from

==========
inprog
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Extensions] Update Detail View UI

We have new mocks!  Update the detail view UI to be more in-line with
the new specs.

Update tests accordingly.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2017-02-28 23:12:52]

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

[commit-bot: I haz the power, 2017-02-28 23:13:18]

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-02-28 23:19:20]

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

[commit-bot: I haz the power, 2017-02-28 23:19:20]

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_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[Devlin, 2017-03-01 00:48:22]

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

[commit-bot: I haz the power, 2017-03-01 00:48:43]

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-03-01 00:57:41]

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

[commit-bot: I haz the power, 2017-03-01 00:57:42]

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

[Devlin, 2017-03-01 01:39:42]

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

[commit-bot: I haz the power, 2017-03-01 01:40:02]

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-03-01 02:51:36]

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

[commit-bot: I haz the power, 2017-03-01 02:51:37]

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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2017-03-01 18:29:39]

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

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

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-03-01 19:40:51]

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

[commit-bot: I haz the power, 2017-03-01 19:40:52]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-03-01 19:47:20]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-03-01 19:47:26]

Patchset #1 (id:20001) has been deleted

[Devlin, 2017-03-01 19:47:33]

Patchset #1 (id:40001) has been deleted

[Devlin, 2017-03-01 19:55:04]

Patchset #1 (id:60001) has been deleted

[Devlin, 2017-03-01 19:55:11]

Patchset #1 (id:80001) has been deleted

[Devlin, 2017-03-01 19:57:58]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Devlin, 2017-03-01 19:57:59]

Michael, mind taking a look?

[michaelpg, 2017-03-01 23:42:09]

On 2017/03/01 19:57:59, Devlin wrote:
> Michael, mind taking a look?

Hey, unfortunately I'm pretty swamped and trying to prioritize today/tomorrow.
Are you aiming to have this ready for the branch tomorrow?

[Devlin, 2017-03-01 23:50:41]

On 2017/03/01 23:42:09, michaelpg wrote:
> On 2017/03/01 19:57:59, Devlin wrote:
> > Michael, mind taking a look?
> 
> Hey, unfortunately I'm pretty swamped and trying to prioritize today/tomorrow.
> Are you aiming to have this ready for the branch tomorrow?

Nope.  Monday's fine.

[michaelpg, 2017-03-01 23:52:02]

On 2017/03/01 23:50:41, Devlin wrote:
> On 2017/03/01 23:42:09, michaelpg wrote:
> > On 2017/03/01 19:57:59, Devlin wrote:
> > > Michael, mind taking a look?
> > 
> > Hey, unfortunately I'm pretty swamped and trying to prioritize
today/tomorrow.
> > Are you aiming to have this ready for the branch tomorrow?
> 
> Nope.  Monday's fine.

Cool, I'll definitely get this to you by then. (Or, of course, feel free to
find another reviewer if you'd prefer.) Thanks!

[michaelpg, 2017-03-06 19:08:47]

As promised.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.html (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:125: <img alt="icon"
id="icon" src="[[data.iconUrl]]">
alt should be localized or ignored (alt="" makes sense IMO:
https://www.w3.org/WAI/tutorials/images/decorative/)

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:228:
hidden$="[[!shouldShowExtensionsOptionsSection_(data.*)]]">
nit: maybe shouldShowOptionsLink_, to disambiguate with
shouldShowOptionsSection_ above? (instead of using Extensions, which doesn't
help me understand the difference between these two methods at a glance)

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:237:
id="remove-extension">
general, optional nit: id, class and is tend to go before other attributes,
especially Polymer made-up ones (I only mention it because this CL isn't
consistent with itself, but I don't care much either way)

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.js:24: inDevMode: Boolean,
I CC'd you on a 30-comment bug thread yesterday where users and testers happened
to be using "dev mode" to mean different things (dev build vs. extensions page
dev mode), and that's excluding Chrome OS's "dev mode" too. Just a warning about
that keyword :-)

As a suggestion, "the UI's developer mode" would be unambiguous.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.js:78: * @param
{chrome.developerPrivate.ExtensionView} view
non-null: !

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/item_util.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017 (looks like more than a straight rename)

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:18: * @param
{chrome.developerPrivate.ExtensionState} state
!

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:29: assertNotReached();  //
FileNotFound.
FileNotFound?

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:35: * @param
{chrome.developerPrivate.ExtensionInfo} item
!

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:60: * @param
{chrome.developerPrivate.ExtensionInfo} item
!

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:96: * @param
{chrome.developerPrivate.ExtensionView} view
non-null: !

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:108: label +=
(view.incognito ? ' ' +
this would be more readable as a series of if's

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/extensions/extension_detail_view_test.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/extensions/extension_detail_view_test.js:114: //
TODO(devlin): Add these back once homepages are in the mocks again.
I would leave a TODO remove the unused code. Yay version control!

[michaelpg, 2017-03-06 19:55:02]

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/extensions/extension_detail_view_test.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/extensions/extension_detail_view_test.js:114: //
TODO(devlin): Add these back once homepages are in the mocks again.
On 2017/03/06 19:08:46, michaelpg wrote:
> I would leave a TODO remove the unused code. Yay version control!

That is, leave this TODO, *but* remove commented-out code.

[Devlin, 2017-03-06 20:12:13]

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.html (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:125: <img alt="icon"
id="icon" src="[[data.iconUrl]]">
On 2017/03/06 19:08:46, michaelpg wrote:
> alt should be localized or ignored (alt="" makes sense IMO:
> https://www.w3.org/WAI/tutorials/images/decorative/)

Whoops, meant to be alt="".  Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:228:
hidden$="[[!shouldShowExtensionsOptionsSection_(data.*)]]">
On 2017/03/06 19:08:46, michaelpg wrote:
> nit: maybe shouldShowOptionsLink_, to disambiguate with
> shouldShowOptionsSection_ above? (instead of using Extensions, which doesn't
> help me understand the difference between these two methods at a glance)

I like it, done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.html:237:
id="remove-extension">
On 2017/03/06 19:08:46, michaelpg wrote:
> general, optional nit: id, class and is tend to go before other attributes,
> especially Polymer made-up ones (I only mention it because this CL isn't
> consistent with itself, but I don't care much either way)

Fair enough; done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.js:24: inDevMode: Boolean,
On 2017/03/06 19:08:46, michaelpg wrote:
> I CC'd you on a 30-comment bug thread yesterday where users and testers
happened
> to be using "dev mode" to mean different things (dev build vs. extensions page
> dev mode), and that's excluding Chrome OS's "dev mode" too. Just a warning
about
> that keyword :-)
> 
> As a suggestion, "the UI's developer mode" would be unambiguous.

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/item_util.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/03/06 19:08:46, michaelpg wrote:
> 2017 (looks like more than a straight rename)

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:18: * @param
{chrome.developerPrivate.ExtensionState} state
On 2017/03/06 19:08:46, michaelpg wrote:
> !

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:29: assertNotReached();  //
FileNotFound.
On 2017/03/06 19:08:46, michaelpg wrote:
> FileNotFound?

Bad joke (that was in the last file):
http://thedailywtf.com/articles/What_Is_Truth_0x3f_

Basically, an enum value that doesn't/shouldn't exist.  Removed, since I can see
how it would be confusing. :)

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:35: * @param
{chrome.developerPrivate.ExtensionInfo} item
On 2017/03/06 19:08:46, michaelpg wrote:
> !

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:60: * @param
{chrome.developerPrivate.ExtensionInfo} item
On 2017/03/06 19:08:46, michaelpg wrote:
> !

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:96: * @param
{chrome.developerPrivate.ExtensionView} view
On 2017/03/06 19:08:46, michaelpg wrote:
> non-null: !

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:108: label +=
(view.incognito ? ' ' +
On 2017/03/06 19:08:46, michaelpg wrote:
> this would be more readable as a series of if's

Done.

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/extensions/extension_detail_view_test.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/extensions/extension_detail_view_test.js:114: //
TODO(devlin): Add these back once homepages are in the mocks again.
On 2017/03/06 19:08:46, michaelpg wrote:
> I would leave a TODO remove the unused code. Yay version control!

Okay, done.

[michaelpg, 2017-03-06 22:19:58]

lgtm

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/item_util.js (right):

https://codereview.chromium.org/2723613004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/item_util.js:29: assertNotReached();  //
FileNotFound.
On 2017/03/06 20:12:12, Devlin wrote:
> On 2017/03/06 19:08:46, michaelpg wrote:
> > FileNotFound?
> 
> Bad joke (that was in the last file):
> http://thedailywtf.com/articles/What_Is_Truth_0x3f_
> 
> Basically, an enum value that doesn't/shouldn't exist.  Removed, since I can
see
> how it would be confusing. :)

Ah. Hehe, and it works on two levels because we'd also return |undefined| in
that case instead of true or false.

https://codereview.chromium.org/2723613004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.js (right):

https://codereview.chromium.org/2723613004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.js:23: /** Whether the user
has the UI's developer mode. */
I probably meant:
  "Whether the user has toggled the UI's developer mode."
                        ^^^^^^^

(Or better, enabled, but now I'm being overly nit-picky.)

[Devlin, 2017-03-06 23:22:36]

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

[commit-bot: I haz the power, 2017-03-06 23:23:37]

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-03-07 01:13:26]

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

[commit-bot: I haz the power, 2017-03-07 01:13:27]

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

[Devlin, 2017-03-07 01:18:47]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-03-07 01:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2723613004/#ps160001
(title: "nit")

[Devlin, 2017-03-07 01:18:56]

https://codereview.chromium.org/2723613004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/detail_view.js (right):

https://codereview.chromium.org/2723613004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/detail_view.js:23: /** Whether the user
has the UI's developer mode. */
On 2017/03/06 22:19:57, michaelpg wrote:
> I probably meant:
>   "Whether the user has toggled the UI's developer mode."
>                         ^^^^^^^
> 
> (Or better, enabled, but now I'm being overly nit-picky.)

Heh, whoops.  Didn't mean to copy-paste the grammatical error, too. :)  Done.

[commit-bot: I haz the power, 2017-03-07 01:19:47]

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-03-07 03:43:54]

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

[commit-bot: I haz the power, 2017-03-07 03:43:55]

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

[Devlin, 2017-03-07 04:35:26]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-03-07 04:36:05]

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-03-07 06:43:18]

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

[commit-bot: I haz the power, 2017-03-07 06:43:20]

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

[Devlin, 2017-03-07 15:49:15]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:50:11]

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-03-07 16:55:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488901755610930,
"parent_rev": "21ff99006de0967a799ee4bf70766a2825044d10", "commit_rev":
"7615c37d20b7b4a57cf9b4ae7e3034e63003b2ec"}

[commit-bot: I haz the power, 2017-03-07 16:56:32]

Description was changed from

==========
[MD Extensions] Update Detail View UI

We have new mocks!  Update the detail view UI to be more in-line with
the new specs.

Update tests accordingly.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Extensions] Update Detail View UI

We have new mocks!  Update the detail view UI to be more in-line with
the new specs.

Update tests accordingly.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2723613004
Cr-Commit-Position: refs/heads/master@{#455099}
Committed:
https://chromium.googlesource.com/chromium/src/+/7615c37d20b7b4a57cf9b4ae7e30...
==========

[commit-bot: I haz the power, 2017-03-07 16:56:34]

Committed patchset #4 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7615c37d20b7b4a57cf9b4ae7e30...