Convert chrome://extensions to a settings page within the options pages.

Convert chrome://extensions to a settings page within the options pages.

BUG=87378
TEST=The new settings page (chrome://settings/extensionSettings) should work the same as the old one (chrome://extensions).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99402

[Finnur, 2011-08-30 20:14:00]

Aaron, this is mostly a continuation of what you reviewed last time, except now
following Glen's design...

I have not removed the old page, so we can compare.

[Aaron Boodman, 2011-08-31 18:22:44]

You should get someone who did work on the settings UI to review the
html/css/js. I don't know enough about it to say anything intelligent.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:75: <!--if expr="not
pp_ifdef('touchui')"-->
Remove dead code?

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
chrome/browser/resources/options/extension_list.js:21: // Make sure developer
mode section is set correctly as per saved setting.
This is a ridiculously long function. Maybe break it up into logical pieces?

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.cc:373:
DCHECK(extension);
CHECK - you're going to crash on the next line anyway.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.h:212:
ExtensionService* /*const*/ extension_service_;
remove dead code

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:90:
pack_job_->Start();
You need to call pack_job_->ClearClient() in the destructor of this class.

[Finnur, 2011-08-31 20:22:01]

Sure, I'll find someone who knows the Settings pages. 
Thanks for reviewing.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:75: <!--if expr="not
pp_ifdef('touchui')"-->
Woops. That file isn't supposed to be here. I thought I had removed it, guess
not.

On 2011/08/31 18:22:44, Aaron Boodman wrote:
> Remove dead code?

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
chrome/browser/resources/options/extension_list.js:21: // Make sure developer
mode section is set correctly as per saved setting.
On 2011/08/31 18:22:44, Aaron Boodman wrote:
> This is a ridiculously long function. Maybe break it up into logical pieces?

Done.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.cc:373:
DCHECK(extension);
Ah, this is from the original extensions_ui.cc code and something I ask people
to remove as well. Serves me right. :)

Removed.

On 2011/08/31 18:22:44, Aaron Boodman wrote:
> CHECK - you're going to crash on the next line anyway.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.h:212:
ExtensionService* /*const*/ extension_service_;
On 2011/08/31 18:22:44, Aaron Boodman wrote:
> remove dead code

Done.

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:90:
pack_job_->Start();
Good catch!

On 2011/08/31 18:22:44, Aaron Boodman wrote:
> You need to call pack_job_->ClearClient() in the destructor of this class.

[Finnur, 2011-08-31 20:24:08]

csilv, can you take a look at the new Settings page?
I'll send you a screenshot from Glen in the mail.

On 2011/08/31 20:22:01, Finnur wrote:
> Sure, I'll find someone who knows the Settings pages. 
> Thanks for reviewing.
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
> File chrome/browser/browser_resources.grd (right):
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/browser_resou...
> chrome/browser/browser_resources.grd:75: <!--if expr="not
> pp_ifdef('touchui')"-->
> Woops. That file isn't supposed to be here. I thought I had removed it, guess
> not.
> 
> On 2011/08/31 18:22:44, Aaron Boodman wrote:
> > Remove dead code?
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
> File chrome/browser/resources/options/extension_list.js (right):
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/resources/opt...
> chrome/browser/resources/options/extension_list.js:21: // Make sure developer
> mode section is set correctly as per saved setting.
> On 2011/08/31 18:22:44, Aaron Boodman wrote:
> > This is a ridiculously long function. Maybe break it up into logical pieces?
> 
> Done.
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/extension_settings_handler.cc:373:
> DCHECK(extension);
> Ah, this is from the original extensions_ui.cc code and something I ask people
> to remove as well. Serves me right. :)
> 
> Removed.
> 
> On 2011/08/31 18:22:44, Aaron Boodman wrote:
> > CHECK - you're going to crash on the next line anyway.
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/extension_settings_handler.h (right):
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/extension_settings_handler.h:212:
> ExtensionService* /*const*/ extension_service_;
> On 2011/08/31 18:22:44, Aaron Boodman wrote:
> > remove dead code
> 
> Done.
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):
> 
>
http://codereview.chromium.org/7794023/diff/7001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/pack_extension_handler.cc:90:
> pack_job_->Start();
> Good catch!
> 
> On 2011/08/31 18:22:44, Aaron Boodman wrote:
> > You need to call pack_job_->ClearClient() in the destructor of this class.

[csilv, 2011-09-01 04:34:09]

Sorry for the delay Finnur, I'll have this review complete first thing in the
morning.

[Finnur, 2011-09-01 16:15:33]

Thanks for the heads up. Appreciate a quick response, since time is of the
essence now (given the timelag due to me being on GMT, more or less).

[csilv, 2011-09-01 19:52:53]

Partial code review... I'll followup with the rest.

cc jhawkins, estade FYI.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:30: InitControlsAndHandlers_:
function() {
style nit: lower-case method name, initControlAndHandlers_

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:30: InitControlsAndHandlers_:
function() {
Add jsdoc comments for this method and the next two.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:37:
toggleButton.setAttribute("checked", "");
could we use toggleButton.checked = true/false instead?

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:37:
toggleButton.setAttribute("checked", "");
style nit: use single quotes for strings, here and below.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:51:
DeleteExistingExtensionNodes_: function(showingDetails, showingWarning) {
style nit: lower-case method name, deleteExistingExtensionNodes_

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:59: if
(child.className.indexOf('extensionListItemExpanded') >= 0) {
simply to:
if (child.classList.contains('extensionListItemExpanded')) {

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:64: if (!(butterBar === null)
&& butterBar.style.display == "block") {
prefer hidden attribute over display for visibility.  ie: butterBar.hidden ==
true

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:73: ShowExtensionNodes_:
function(showingDetails, showingWarning) {
style nit: lower-case method name, showExtensionNodes_

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:107: if (!extension.enabled)
{
style nit: no need for brackets for single line, here and below.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:217:
input.setAttribute("disabled", "");
input.disabled = true/false (here and below)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:220:
input.setAttribute("checked", "");
input.checked = true/false (here and below)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:262:
label.setAttribute("style", "margin-right: " +
lable.style.marginRight = ...

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:352: content =
this.ownerDocument.createElement('div');
var content =

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:477: var zippy =
this.ownerDocument.getElementById(iter.id + '_zippy');
var zippy = $(iter.id + '_zippy');

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:478: var details =
this.ownerDocument.getElementById(iter.id + '_details');
var details = $(iter.id + '_details');

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:494: butterBar.style.display
= "none";
butterBar.hidden = true;

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:521:
document.getElementById('search-field').value.toLowerCase();
$('search-field').value.toLowerCase();

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:620: if
(butterBar.style.display == "none") {
butterBar.hidden = true/false (here and below)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:7: a {
This file is going to be pulled into the options window, so keep in mind that
there is always a danger of name collisions.  In this case, these link tags will
(potentially) affect all links in the options window.  Do we need links to have
a different behavior/appearance than the rest of the options tab?

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:23: .hbox {
We should consider moving thse (.hbox, .vbox, .stretch) to a more general
location like options_page.css, or even more general within the shared
directory.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:38: .extensionListItem {
use dash naming for "extensionList" elements.  (ie, extension-list-item,
extension-list-item-collapsed, extension-list-item-expanded).

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:39: font-family:
'Helvetica Neue', Arial, sans-serif;
Remove font-family setting, the font will be specified by the options window.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:49: height: Auto;
nit: lowercase 'auto'

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:72: border-bottom : 0px
solid #eeeeee;
nit: color as #eee

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:88: min-height: 0px;
min-height: 0;

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:93: height: 0px;
height: 0;

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:112: width: 48px;
order properties in alphabetical order (here and below)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:114: padding-left: 15px;
Perhaps not for this CL, but we should consider RTL friendly properties, ex
-webkit-padding-start and -webkit-padding-end rather than padding-left,
padding-right. (These are okay since they're identical, but there are other
left/right padding and margin attributes below.)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:175: .gray-text {
nit: ideally the name of this is more to convey why it's gray... so
'informative-text' or 'supplmental-text', etc... as maybe in the future we'll
want that informative text to be a different color.  :-)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:5: <label title="Toggle
developer mode" for="toggle-dev-on"
The title should be localized.  Instead of this attribute, use:
  i18n-values="title:toggleDeveloperMode"

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:13: <button
id="load-unpacked">Load unpacked extension...</button>
Use i18n-content to localize strings, here and below.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:29: <div><strong
id="no-extensions"
set the 'hidden' attribute rather than the style tag.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:34: <image
src="chrome://theme/IDR_WEBSTORE_ICON_32" />
don't need the trailing slash for image tags w/ HTML5.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/options_ui_uitest.cc (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/options_ui_uitest.cc:154: //   users and
extensions.
For consistency, upper case "Users" and "Extensions"

[csilv, 2011-09-01 21:10:45]

Here's the rest of my code review.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:43:
localStrings.getString('suggestGallery');
This can be done more concisely by setting the i18n-content attribute in the
HTML file.  (here and for the next line.)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:62: */
@private

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:112: if
(dev.className.indexOf('dev-open') == -1) {
dev.classList.contains

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:145:
$('no-extensions').style.display = "none";
use hidden attribute, here and below

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/pack_extension_overlay.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/pack_extension_overlay.js:51: *
operation-specific configuration.
@private

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:181: void
ExtensionSettingsHandler::RegisterMessages() {
Some of these message names are very general (ie: "reload", "enable", etc). 
Since these message callbacks are registered in the same namespace as the entire
options window, these could easily conflict.  I'd be good to prefix all of these
messages with "extensionSettings".

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:586:
RegisterTitle(localized_strings, "extensionSettings",
Same comment as above with registereind message names.  localized_strings are
shared for the entire options window.  Ideally the general once here like
"enabled", would have a unique prefix.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:171: //
DOMMessageHandler implementation.
This is a WebUIMessageHandler override, could group it with RegisterMesages()
above.

[Finnur, 2011-09-02 13:58:47]

Phew. Addressed the comments!
PTAL.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:30: InitControlsAndHandlers_:
function() {
On 2011/09/01 19:52:53, csilv wrote:
> style nit: lower-case method name, initControlAndHandlers_

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:37:
toggleButton.setAttribute("checked", "");
On 2011/09/01 19:52:53, csilv wrote:
> style nit: use single quotes for strings, here and below.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:51:
DeleteExistingExtensionNodes_: function(showingDetails, showingWarning) {
On 2011/09/01 19:52:53, csilv wrote:
> style nit: lower-case method name, deleteExistingExtensionNodes_

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:59: if
(child.className.indexOf('extensionListItemExpanded') >= 0) {
On 2011/09/01 19:52:53, csilv wrote:
> simply to:
> if (child.classList.contains('extensionListItemExpanded')) {

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:64: if (!(butterBar === null)
&& butterBar.style.display == "block") {
On 2011/09/01 19:52:53, csilv wrote:
> prefer hidden attribute over display for visibility.  ie: butterBar.hidden ==
> true

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:73: ShowExtensionNodes_:
function(showingDetails, showingWarning) {
On 2011/09/01 19:52:53, csilv wrote:
> style nit: lower-case method name, showExtensionNodes_

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:107: if (!extension.enabled)
{
On 2011/09/01 19:52:53, csilv wrote:
> style nit: no need for brackets for single line, here and below.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:217:
input.setAttribute("disabled", "");
On 2011/09/01 19:52:53, csilv wrote:
> input.disabled = true/false (here and below)

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:220:
input.setAttribute("checked", "");
On 2011/09/01 19:52:53, csilv wrote:
> input.checked = true/false (here and below)

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:262:
label.setAttribute("style", "margin-right: " +
On 2011/09/01 19:52:53, csilv wrote:
> lable.style.marginRight = ...

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:352: content =
this.ownerDocument.createElement('div');
Seriously? :) Don't tell my you found this without applying my patch? :)

Anyway, done.


On 2011/09/01 19:52:53, csilv wrote:
> var content =

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:477: var zippy =
this.ownerDocument.getElementById(iter.id + '_zippy');
On 2011/09/01 19:52:53, csilv wrote:
> var zippy = $(iter.id + '_zippy');

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:478: var details =
this.ownerDocument.getElementById(iter.id + '_details');
On 2011/09/01 19:52:53, csilv wrote:
> var details = $(iter.id + '_details');

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:494: butterBar.style.display
= "none";
On 2011/09/01 19:52:53, csilv wrote:
> butterBar.hidden = true;

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:521:
document.getElementById('search-field').value.toLowerCase();
On 2011/09/01 19:52:53, csilv wrote:
> $('search-field').value.toLowerCase();

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:620: if
(butterBar.style.display == "none") {
On 2011/09/01 19:52:53, csilv wrote:
> butterBar.hidden = true/false (here and below)

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:7: a {
Hmm... probably best to keep the look the same as the other links (or change it
wholesale).

Removed.


On 2011/09/01 19:52:53, csilv wrote:
> This file is going to be pulled into the options window, so keep in mind that
> there is always a danger of name collisions.  In this case, these link tags
will
> (potentially) affect all links in the options window.  Do we need links to
have
> a different behavior/appearance than the rest of the options tab?

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:23: .hbox {
Mind if I submit it as a followup CL? 

On 2011/09/01 19:52:53, csilv wrote:
> We should consider moving thse (.hbox, .vbox, .stretch) to a more general
> location like options_page.css, or even more general within the shared
> directory.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:38: .extensionListItem {
On 2011/09/01 19:52:53, csilv wrote:
> use dash naming for "extensionList" elements.  (ie, extension-list-item,
> extension-list-item-collapsed, extension-list-item-expanded).

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:39: font-family:
'Helvetica Neue', Arial, sans-serif;
On 2011/09/01 19:52:53, csilv wrote:
> Remove font-family setting, the font will be specified by the options window.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:49: height: Auto;
So... not German, eh? ;)
Done.

On 2011/09/01 19:52:53, csilv wrote:
> nit: lowercase 'auto'

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:72: border-bottom : 0px
solid #eeeeee;
On 2011/09/01 19:52:53, csilv wrote:
> nit: color as #eee

Ah... so not so big on Euros, either, eh? Must be American. ;)
Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:88: min-height: 0px;
On 2011/09/01 19:52:53, csilv wrote:
> min-height: 0;

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:93: height: 0px;
On 2011/09/01 19:52:53, csilv wrote:
> height: 0;

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:112: width: 48px;
On 2011/09/01 19:52:53, csilv wrote:
> order properties in alphabetical order (here and below)

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:114: padding-left: 15px;
Ah, not so friendly with the RTL people, eh? Oh, wait... that would be me.
Noted. :)

On 2011/09/01 19:52:53, csilv wrote:
> Perhaps not for this CL, but we should consider RTL friendly properties, ex
> -webkit-padding-start and -webkit-padding-end rather than padding-left,
> padding-right. (These are okay since they're identical, but there are other
> left/right padding and margin attributes below.)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:175: .gray-text {
On 2011/09/01 19:52:53, csilv wrote:
> nit: ideally the name of this is more to convey why it's gray... so
> 'informative-text' or 'supplmental-text', etc... as maybe in the future we'll
> want that informative text to be a different color.  :-)

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:5: <label title="Toggle
developer mode" for="toggle-dev-on"
This tooltip is kind of useless and obvious. Removed.

On 2011/09/01 19:52:53, csilv wrote:
> The title should be localized.  Instead of this attribute, use:
>   i18n-values="title:toggleDeveloperMode"

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:13: <button
id="load-unpacked">Load unpacked extension...</button>
Ah, good catch!

On 2011/09/01 19:52:53, csilv wrote:
> Use i18n-content to localize strings, here and below.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:29: <div><strong
id="no-extensions"
On 2011/09/01 19:52:53, csilv wrote:
> set the 'hidden' attribute rather than the style tag.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:34: <image
src="chrome://theme/IDR_WEBSTORE_ICON_32" />
On 2011/09/01 19:52:53, csilv wrote:
> don't need the trailing slash for image tags w/ HTML5.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:43:
localStrings.getString('suggestGallery');
Not quite. suggestGallery content is HTML passed down from C++ whereas
i18n-content sets the content as text. Hence, it doesn't get linkified.

On 2011/09/01 21:10:45, csilv wrote:
> This can be done more concisely by setting the i18n-content attribute in the
> HTML file.  (here and for the next line.)

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:62: */
On 2011/09/01 21:10:45, csilv wrote:
> @private

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:112: if
(dev.className.indexOf('dev-open') == -1) {
On 2011/09/01 21:10:45, csilv wrote:
> dev.classList.contains

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:145:
$('no-extensions').style.display = "none";
On 2011/09/01 21:10:45, csilv wrote:
> use hidden attribute, here and below

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/pack_extension_overlay.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/pack_extension_overlay.js:51: *
operation-specific configuration.
On 2011/09/01 21:10:45, csilv wrote:
> @private

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:181: void
ExtensionSettingsHandler::RegisterMessages() {
On 2011/09/01 21:10:45, csilv wrote:
> Some of these message names are very general (ie: "reload", "enable", etc). 
> Since these message callbacks are registered in the same namespace as the
entire
> options window, these could easily conflict.  I'd be good to prefix all of
these
> messages with "extensionSettings".

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:586:
RegisterTitle(localized_strings, "extensionSettings",
On 2011/09/01 21:10:45, csilv wrote:
> Same comment as above with registereind message names.  localized_strings are
> shared for the entire options window.  Ideally the general once here like
> "enabled", would have a unique prefix.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:171: //
DOMMessageHandler implementation.
On 2011/09/01 21:10:45, csilv wrote:
> This is a WebUIMessageHandler override, could group it with RegisterMesages()
> above.

Done.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/options_ui_uitest.cc (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/options_ui_uitest.cc:154: //   users and
extensions.
On 2011/09/01 19:52:53, csilv wrote:
> For consistency, upper case "Users" and "Extensions"

Done.

[csilv, 2011-09-02 17:41:10]

Pretty close, just a few things I'd like you fix.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:23: .hbox {
I don't mind at all, lets do that.

On 2011/09/02 13:58:47, Finnur wrote:
> Mind if I submit it as a followup CL? 
> 
> On 2011/09/01 19:52:53, csilv wrote:
> > We should consider moving thse (.hbox, .vbox, .stretch) to a more general
> > location like options_page.css, or even more general within the shared
> > directory.
>

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.js (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.js:43:
localStrings.getString('suggestGallery');
Ah, thanks for the correction.

On 2011/09/02 13:58:47, Finnur wrote:
> Not quite. suggestGallery content is HTML passed down from C++ whereas
> i18n-content sets the content as text. Hence, it doesn't get linkified.
> 
> On 2011/09/01 21:10:45, csilv wrote:
> > This can be done more concisely by setting the i18n-content attribute in the
> > HTML file.  (here and for the next line.)
>

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:7: .hbox {
As discussed, add TODO to move these to a common location (either
options_page.css or somewhere in the shared directory.)

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:31: list > * {
this item and the next will conflict with other options window lists.  please
change the rule so that it only affects this list.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:54: .content {
Please rename this one, it's just too general of a name for me to feel
comfortable.  :-)  Alternatively, change the rule so that specific to this page.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:70: .page list {
This conflicts with the .page list rule in options_page.css.  Please make it
more specific.

[Finnur, 2011-09-02 18:01:39]

Done.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:7: .hbox {
On 2011/09/02 17:41:11, csilv wrote:
> As discussed, add TODO to move these to a common location (either
> options_page.css or somewhere in the shared directory.)

Done.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:31: list > * {
On 2011/09/02 17:41:11, csilv wrote:
> this item and the next will conflict with other options window lists.  please
> change the rule so that it only affects this list.

Done.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:54: .content {
On 2011/09/02 17:41:11, csilv wrote:
> Please rename this one, it's just too general of a name for me to feel
> comfortable.  :-)  Alternatively, change the rule so that specific to this
page.

Done.

http://codereview.chromium.org/7794023/diff/26001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:70: .page list {
I don't think this was used anymore. Removed.

On 2011/09/02 17:41:11, csilv wrote:
> This conflicts with the .page list rule in options_page.css.  Please make it
> more specific.

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:256: 
This is unrelated to your request, but was missing from last cl.

[csilv, 2011-09-02 18:03:58]

LGTM

[arv (Not doing code reviews), 2011-09-06 20:55:47]

Sorry for not looking at this earlier.

Most of the comments are just nits/style violations.

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7794023/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:34: <image
src="chrome://theme/IDR_WEBSTORE_ICON_32" />
<img>

<image> is an IE compat quirk

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:50:
document.addEventListener('keyup', this.upEventHandler_.bind(this));
Is there a reason why these are added to document?

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:61: *                of
extension that had the details section expanded.
Nit: indent 4 spaces

* @param {Array} showingDetails An array that will contain the list of id's
*     of extension that had the details section expanded.

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:79: if (!(butterBar === null)
&& !butterBar.hidden)
if (butterBar && !butterBar.hidden)

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:98: var minCheckboxWidth =
999999;
or maybe 

var minCheckboxWidth = Infinity;

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:102: for (var i = 0; i <
this.data_.extensions.length; ++i) {
prefer i++ for consistency

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:135: var vbox_outer =
this.ownerDocument.createElement('div');
no underscores in JS

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:152: div =
this.ownerDocument.createElement('div');
missing var

consider using strict mode to get warnings about this

cr.define('options', function() {
  'use strict';

  ...
});

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:160: icon =
this.ownerDocument.createElement('img');
missing var

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:213:
localStrings.getString('extensionSettingsVisitWebsite');
indentation

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:239: if
(!extension.mayDisable)
or 

input.disabled = !extension.mayDisable;

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:249:
label.setAttribute('for', 'toggle-' + id);
label.htmlFor = ...

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:261: if (label.offsetWidth >
maxCheckboxWidth)
Can you move this to the end to prevent extra layouts?

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:263: if (label.offsetWidth <
minCheckboxWidth)
same with this one

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:283: if (label.offsetWidth <
maxCheckboxWidth)
offsetWidth causes layout which are slow. You might want to defer this to later

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:284: label.style.marginRight
= difference.toString() + 'px';
label.style.WebkitMarginEnd = ...

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:301: var details_contents =
this.ownerDocument.createElement('div');
detailsContents

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:318:
localStrings.getString('extensionSettingsExtensionId') +
indentation

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:400:
content.appendChild(link);
This looks like copy pasted code. Can you create a function?

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:462: $(zippy).style.display =
(itemsShown > 0) ? 'block' : 'none';
no parentheses

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:462: $(zippy).style.display =
(itemsShown > 0) ? 'block' : 'none';
Can you use $(zippy).hidden = ...?

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:471: for (var i = 0; i <
this.data_.extensions.length; ++i) {
indexOf?

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:484: findIdNode_:
function(node) {
move off prototype? If you do not depend on this in a private method consider
making it a function instead.

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:485: while (node.id.length ==
0) {
while (node && !node.id) {
  node = node.parentNode;
}
return node;

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:509:
zippy.classList.remove('extension-zippy-expanded');
this is fine for this patch but why do we have a class for both expanded and
collapsed. Only using one would simplify things

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:517: var butterBar =
this.ownerDocument.getElementById(
or

this.querySelector('#' + iter.id + '_incognitoWarning')

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.css (right):

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.css:111: padding-right: 5px;
-webkit-padding-end?

http://dev.chromium.org/developers/web-development-style-guide

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_settings.html (right):

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/extension_settings.html:36: <image
src="chrome://theme/IDR_WEBSTORE_ICON_32">
<img>

<image> is an IE compat quirk

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
File chrome/browser/resources/options/pack_extension_overlay.js (right):

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/pack_extension_overlay.js:11: * @class
@constructor

http://codereview.chromium.org/7794023/diff/26005/chrome/browser/resources/op...
chrome/browser/resources/options/pack_extension_overlay.js:52: @private
* @private