Show extension name for dependent extensions

Show extension name for dependent extensions

* show ID when "Developer Mode" is checked

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

[elijahtaylor1, 2014-08-12 23:19:12]

kalman:  I was hoping to improve the shared modules messaging by including their
user readable name, and ID only in dev mode (to match how extensions are
displayed normally).  WDYT?

[not at google - send to devlin, 2014-08-13 01:42:31]

https://codereview.chromium.org/462273003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/462273003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_list.js:329: idSpan.innerText = '
(' + elem.id + ')';
You should use .textContent not .innerText.

Anyhow, showing the extension's name seems like the right thing to do. You could
imagine showing more information than that (I imagine we will want to show
things like this about_page thing at some point? what ever happened to that).

I'm wondering whether you should define a template for this, even though it's
only a couple of things, it might be good practice.

See how template are defined in here and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

[elijahtaylor1, 2014-08-13 18:02:59]

PTAL

https://codereview.chromium.org/462273003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/462273003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_list.js:329: idSpan.innerText = '
(' + elem.id + ')';
On 2014/08/13 01:42:31, kalman wrote:
> You should use .textContent not .innerText.
> 
> Anyhow, showing the extension's name seems like the right thing to do. You
could
> imagine showing more information than that (I imagine we will want to show
> things like this about_page thing at some point? what ever happened to that).
> 
> I'm wondering whether you should define a template for this, even though it's
> only a couple of things, it might be good practice.
> 
> See how template are defined in here and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....


great, a template is what I was looking for (instead of adding class names and
structure in JS)

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.html:256: <li
class="dependent-list-item">
I wasn't sure if I should continue to use <li> or not, the other "list" is just
a series of divs, but I'm actually using an <ul>

[not at google - send to devlin, 2014-08-13 19:43:47]

lgtm, nits left.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:326:
'.dependent-list-item');
Save this outside the forEach loop?

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:329:
dependentTitle.textContent = elem.name;
Nit: to save on the saturation of "dependent*" variables here could you do that
inline? node.querySelector('.extension-title').textContent = elem.name; etc.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.html:255: <div
id="template-dependent" hidden>
I'm not at all familiar with the idioms of this file, but it does look like the
other templates are declared inside that template-collection <div>.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.html:256: <li
class="dependent-list-item">
On 2014/08/13 18:02:59, elijahtaylor1 wrote:
> I wasn't sure if I should continue to use <li> or not, the other "list" is
just
> a series of divs, but I'm actually using an <ul>

<li> looks right, but having <divs> inside <li> is odd since div is a block and
li is a line. Use <span> instead?

[elijahtaylor1, 2014-08-13 21:12:46]

I'm running tryjobs before checking CQ in case you want to chime in on my edits

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:326:
'.dependent-list-item');
On 2014/08/13 19:43:47, kalman wrote:
> Save this outside the forEach loop?

Done.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:329:
dependentTitle.textContent = elem.name;
On 2014/08/13 19:43:47, kalman wrote:
> Nit: to save on the saturation of "dependent*" variables here could you do
that
> inline? node.querySelector('.extension-title').textContent = elem.name; etc.

Done.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.html:255: <div
id="template-dependent" hidden>
On 2014/08/13 19:43:47, kalman wrote:
> I'm not at all familiar with the idioms of this file, but it does look like
the
> other templates are declared inside that template-collection <div>.

Done.

https://codereview.chromium.org/462273003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.html:256: <li
class="dependent-list-item">
On 2014/08/13 19:43:47, kalman wrote:
> On 2014/08/13 18:02:59, elijahtaylor1 wrote:
> > I wasn't sure if I should continue to use <li> or not, the other "list" is
> just
> > a series of divs, but I'm actually using an <ul>
> 
> <li> looks right, but having <divs> inside <li> is odd since div is a block
and
> li is a line. Use <span> instead?

I got rid of the divs and used an embedded <ul> to deal with displaying the ID
(and potentially other developer mode data eventually)

[elijahtaylor1, 2014-08-13 23:41:37]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-08-13 23:45:38]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/462273003/40001

[commit-bot: I haz the power, 2014-08-14 09:48:15]

Committed patchset #3 (40001) as 289504