Displaying the extensions count in extensions settings page.

Displaying the extensions count in extensions settings page.
BUG=96855
TEST=Load chrome://settings/extensionSettings and see that the title format is now Extensions (xx) where xx is the number of installed extensions in the list. Also if there are no extensions loaded then the title should be Extensions.

Patch by naveenbobbili@motorola.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102101

[Finnur, 2011-09-19 13:49:56]

General review comment: 

CL description: nit: "Fix for issue:96855" is redundant.
CL description: nit: Missing spaces after periods and before open parenthesis.

From looking at the changelist, I suspect it would also be beneficial for you to
review the style guides, since this is your first changelist and you may be
familiar with a different style from your previous work.

See:
http://www.chromium.org/developers/web-development-style-guide
http://www.chromium.org/developers/coding-style

http://codereview.chromium.org/7932021/diff/1/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/7932021/diff/1/AUTHORS#newcode126
AUTHORS:126: Naveen Bobbili <naveenbobbili@motorola.com>
You signed the individual contributor agreement already?

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.html:3:
style="visibility:hidden"></h1>
prefer: hidden="true" over the style attribute (as in lines 34-36). 

However, I'd prefer if you tried to add a new span inside the <h1>, which would
contain the count and parentheses only. Then you don't need to modify the
visibility of these elements and the count can appear after the page has loaded.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.html:4: <div id="dev-toggle"
style="visibility:hidden">
Why is this needed? Is this so it doesn't appear at the top and then move down
as the header becomes visible?

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:157: // the extension
header string .If there are no extensions loaded
nit: Space is on the wrong side of the period.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:158: //  then display
Title as Extensions only.
nit: Title should not be capitalized.

Actually, this can be reworded and shortened to: Show the extensions count (if
non-zero) in the header.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:162:
if(extensionsData.extensions.length > 0) {
style: Space after 'if' (before parentheses).

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:166: + ")";
style: When you need to break long lines into multiple, the operators should
usually end the line, not begin them (move the plus up a line).

Also, the indentation is wrong. Either line the strings up on one line (up
against templateData.extensionSettingsTitle) or line them up 4 spaces to the
right of extensionSetttingsHeader.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:168: else
style: I believe if the 'if' clause has braces, the else should have it too.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:171:
$('extension-settings-header').innerHTML = extensionSettingsHeader;
use innerText, not innerHTML.

[naveenbobbili, 2011-09-19 14:43:18]

http://codereview.chromium.org/7932021/diff/1/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/7932021/diff/1/AUTHORS#newcode126
AUTHORS:126: Naveen Bobbili <naveenbobbili@motorola.com>
On 2011/09/19 13:49:56, Finnur wrote:
> You signed the individual contributor agreement already? 

Yes i did

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.html:4: <div id="dev-toggle"
style="visibility:hidden">
On 2011/09/19 13:49:56, Finnur wrote:
> Why is this needed? Is this so it doesn't appear at the top and then move down
> as the header becomes visible?

Yes this comes in the same line as the title. So it will be weird to show it
without showing the title. I hope I am clear.

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7932021/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_settings.js:171:
$('extension-settings-header').innerHTML = extensionSettingsHeader;
On 2011/09/19 13:49:56, Finnur wrote:
> use innerText, not innerHTML.
Sure. I will take a look at the styling guide lines and submit the patch with
your comments changes.

[naveenbobbili, 2011-09-19 14:45:41]

Uploaded patch set 2 with fixes for all the review comments. I have already
signed the license agreement. Please let me know in case I need any further
details.

[Finnur, 2011-09-19 16:48:41]

Comments not addressed:

CL description: nit: "Fix for issue:96855" is redundant (you already state the
bug number below). You can remove it.
CL description: nit: Missing space (still one) after period and before open
parenthesis.

http://codereview.chromium.org/7932021/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7932021/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/extension_settings.js:165: ")";
nit: Move this to the line above.

[naveenbobbili, 2011-09-19 17:03:34]

Uploaded patchset 3 with review comment changes.

[naveenbobbili, 2011-09-21 02:17:07]

Hi
Can you please check these changes in ?
Yo might see a conflict in AUTHORS file .Please let me know incase you have any
further information.

Regards,
Naveen Bobbili

[Finnur, 2011-09-21 10:14:35]

LGTM.

On 2011/09/21 02:17:07, naveenbobbili wrote:
> Hi
> Can you please check these changes in ?
> Yo might see a conflict in AUTHORS file .Please let me know incase you have
any
> further information.
> 
> Regards,
> Naveen Bobbili

[Finnur, 2011-09-21 10:53:20]

I wanted to try it out first in my local tree to see how it felt (I meant to do
this yesterday, but got sidetracked) and when I did I didn't see a problem with
this change. I've now checked the commit box, which _should_ commit this change
directly (I think).

[commit-bot: I haz the power, 2011-09-21 13:32:13]

Change committed as 102101