Further polish to extensions_ui page

Further polish to extensions_ui page.

This changes the details text from gray to black. It also persists in the preferences the state of devMode (whether the developer tools are showing). Lastly, it adds a fade/slide effect on page elements which are shown in devMode.

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

[Aaron Boodman, 2009-10-26 02:08:34]

Where is the part where the initial state of the pref is added to the JSON?

http://codereview.chromium.org/329018/diff/1/2
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/329018/diff/1/2#newcode1283
Line 1283: prefs->RegisterBooleanPref(prefs::kExtensionsUIDeveloperMode, false);
This is an odd place to register this pref. Can it go in the ExtensionsUI class
instead?

[Finnur, 2009-10-26 02:47:45]

One general questions for this CL and a few nits: 

Why implement this as a pref when it can be done in 5 lines of code using
localStorage?

http://codereview.chromium.org/329018/diff/1/5
File chrome/browser/resources/extensions_ui.html (left):

http://codereview.chromium.org/329018/diff/1/5#oldcode114
Line 114: color: gray;
You dare go against the almighty graphics designer!?

;)

http://codereview.chromium.org/329018/diff/1/5
File chrome/browser/resources/extensions_ui.html (right):

http://codereview.chromium.org/329018/diff/1/5#newcode82
Line 82: height: 0 !important;
Where does this 'important' variable come from? It doesn't seem to be defined
anywhere?

http://codereview.chromium.org/329018/diff/1/5#newcode317
Line 317: * Toggles the devToolsExpanded, and notifys the c++ dom_ui to toggle
the
nit: notifies

http://codereview.chromium.org/329018/diff/1/5#newcode375
Line 375: }
nit: don´t need the braces. Same below.

http://codereview.chromium.org/329018/diff/1/5#newcode575
Line 575: <div id="top" class="wbox"style="padding-right: 10px">
missing space.

[Aaron Boodman, 2009-10-26 03:12:28]

On 2009/10/26 02:47:45, Finnur wrote:
> One general questions for this CL and a few nits: 
> 
> Why implement this as a pref when it can be done in 5 lines of code using
> localStorage?

I thought localStorage wasn't implemented for chrome:// URLs. If it can be
without too much fuss, then all the better.

> http://codereview.chromium.org/329018/diff/1/5#newcode82
> Line 82: height: 0 !important;
> Where does this 'important' variable come from? It doesn't seem to be defined
> anywhere?

It's a built-in CSS thing.

[rafaelw, 2009-10-26 03:57:02]

> Where is the part where the initial state of the pref is added to the JSON?

extension_ui.cc:141-143

http://codereview.chromium.org/329018/diff/1/2
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/329018/diff/1/2#newcode1283
Line 1283: prefs->RegisterBooleanPref(prefs::kExtensionsUIDeveloperMode, false);
Done. Moved to extensions_dom_ui.cc.

On 2009/10/26 02:08:34, Aaron Boodman wrote:
> This is an odd place to register this pref. Can it go in the ExtensionsUI
class
> instead?

http://codereview.chromium.org/329018/diff/1/5
File chrome/browser/resources/extensions_ui.html (left):

http://codereview.chromium.org/329018/diff/1/5#oldcode114
Line 114: color: gray;
On 2009/10/26 02:47:45, Finnur wrote:
> You dare go against the almighty graphics designer!?
> 
> ;)
> 

It's Aaron who dares. It's a part of the bug. I was fine either way =-).

http://codereview.chromium.org/329018/diff/1/5
File chrome/browser/resources/extensions_ui.html (right):

http://codereview.chromium.org/329018/diff/1/5#newcode82
Line 82: height: 0 !important;
!important is a directive to css. When multiple values for the same property can
be applied to an element, !important tells the UA to attempt to prefer it.

Without this, the height value wouldn't take because it is also set on the
element itself.

On 2009/10/26 02:47:45, Finnur wrote:
> Where does this 'important' variable come from? It doesn't seem to be defined
> anywhere?

http://codereview.chromium.org/329018/diff/1/5#newcode317
Line 317: * Toggles the devToolsExpanded, and notifys the c++ dom_ui to toggle
the
On 2009/10/26 02:47:45, Finnur wrote:
> nit: notifies

Done.

http://codereview.chromium.org/329018/diff/1/5#newcode575
Line 575: <div id="top" class="wbox"style="padding-right: 10px">
On 2009/10/26 02:47:45, Finnur wrote:
> missing space.

Done.

[rafaelw1, 2009-10-26 04:01:09]

I kind of got the feeling that localStorage is having problems at the
moment. I can look into enabling it for chrome:// URLs, if you think it's
worthwhile doing.

On Sun, Oct 25, 2009 at 8:12 PM, <aa@chromium.org> wrote:

> On 2009/10/26 02:47:45, Finnur wrote:
>
>> One general questions for this CL and a few nits:
>>
>
>  Why implement this as a pref when it can be done in 5 lines of code using
>> localStorage?
>>
>
> I thought localStorage wasn't implemented for chrome:// URLs. If it can be
> without too much fuss, then all the better.
>
>
>  http://codereview.chromium.org/329018/diff/1/5#newcode82
>> Line 82: height: 0 !important;
>> Where does this 'important' variable come from? It doesn't seem to be
>> defined
>> anywhere?
>>
>
> It's a built-in CSS thing.
>
>
>
>
> http://codereview.chromium.org/329018
>

[Finnur, 2009-10-26 04:30:41]

I had problems with two asserts. An assert (that I think jorlow is fixing this
week) and an assert in the dev tools (that made it hard to debug, so I held
off). I don't remember if I got stopped before verifying that localStorage works
for chrome:// urls..., but if it does work it seems like a lot less work (and
code)... :) (shrug).

On 2009/10/26 04:01:09, rafaelw_google.com wrote:
> I kind of got the feeling that localStorage is having problems at the
> moment. I can look into enabling it for chrome:// URLs, if you think it's
> worthwhile doing.
> 
> On Sun, Oct 25, 2009 at 8:12 PM, <mailto:aa@chromium.org> wrote:
> 
> > On 2009/10/26 02:47:45, Finnur wrote:
> >
> >> One general questions for this CL and a few nits:
> >>
> >
> >  Why implement this as a pref when it can be done in 5 lines of code using
> >> localStorage?
> >>
> >
> > I thought localStorage wasn't implemented for chrome:// URLs. If it can be
> > without too much fuss, then all the better.
> >
> >
> >  http://codereview.chromium.org/329018/diff/1/5#newcode82
> >> Line 82: height: 0 !important;
> >> Where does this 'important' variable come from? It doesn't seem to be
> >> defined
> >> anywhere?
> >>
> >
> > It's a built-in CSS thing.
> >
> >
> >
> >
> > http://codereview.chromium.org/329018
> >
>

[Aaron Boodman, 2009-10-26 04:57:04]

I dare.

LGTM

[Finnur, 2009-10-26 05:21:33]

LG

http://codereview.chromium.org/329018/diff/4002/4006
File chrome/browser/resources/extensions_ui.html (left):

http://codereview.chromium.org/329018/diff/4002/4006#oldcode121
Line 121: display: none;
Just to be sure: The details are still hidden by default, until you toggle,
right?

http://codereview.chromium.org/329018/diff/4002/4006
File chrome/browser/resources/extensions_ui.html (right):

http://codereview.chromium.org/329018/diff/4002/4006#newcode654
Line 654: onclick="sendInspectMessage(this.extensionView); return false;">
nit: This is above 80 columns.

[rafaelw, 2009-10-26 18:23:22]

http://codereview.chromium.org/329018/diff/4002/4006
File chrome/browser/resources/extensions_ui.html (left):

http://codereview.chromium.org/329018/diff/4002/4006#oldcode121
Line 121: display: none;
On 2009/10/26 05:21:33, Finnur wrote:
> Just to be sure: The details are still hidden by default, until you toggle,
> right?

Yes

http://codereview.chromium.org/329018/diff/4002/4006
File chrome/browser/resources/extensions_ui.html (right):

http://codereview.chromium.org/329018/diff/4002/4006#newcode654
Line 654: onclick="sendInspectMessage(this.extensionView); return false;">
On 2009/10/26 05:21:33, Finnur wrote:
> nit: This is above 80 columns.

With html I've been taking a best-effort with 80 cols. In cases where it's 80
cols vs. readable indenting of nested elements, I prefer the later.