Change the view mode when switching between moles and toolstrips, and

Change the view mode when switching between moles and toolstrips, and 
propogate this into the class of the document element so that it's 
possible to use CSS rules to control the display of your toolstrip/mole.

BUG=21939, 15494
TEST=run the Mappy extension and verify it can open and close

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

[Erik does not do reviews, 2009-09-18 00:11:40]

finnur: css injection changes in extension_host, render_view and extension_shelf
aa: everything

[Aaron Boodman, 2009-09-18 00:44:37]

http://codereview.chromium.org/208020/diff/1001/2002
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/208020/diff/1001/2002#newcode210
Line 210: render_view_host()->InsertCSSInWebFrame(L"", css,
"ToolstripThemeCss");
Nit: can we change this string to have "CSS" capitalized?

http://codereview.chromium.org/208020/diff/1001/2003
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/208020/diff/1001/2003#newcode72
Line 72: //
You just blew my mind, dude.

http://codereview.chromium.org/208020/diff/1001/2012
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/208020/diff/1001/2012#newcode164
Line 164: (*it)->view_type() == ViewType::EXTENSION_MOLE)) {
Suggest reworking this logic to make easier to read. Maybe pull out a short
helper function that can peel off each case and return early?

http://codereview.chromium.org/208020/diff/1001/2017
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/208020/diff/1001/2017#newcode119
Line 119: var modeClass = "extension_" + type;
It is more common in CSS to use dash as the word separator.

http://codereview.chromium.org/208020/diff/1001/2017#newcode124
Line 124: classes.forEach(function(cls) {
Heh, if we have .forEach built-in, why are we defining it up above? Can you
convert the usages of that function to the built-in and remove it as long as
you're in here?

http://codereview.chromium.org/208020/diff/1001/2018
File chrome/renderer/resources/extensions_toolstrip.css (right):

http://codereview.chromium.org/208020/diff/1001/2018#newcode15
Line 15: /*color: $TEXT_COLOR$; */
Looks like this can just be removed?

http://codereview.chromium.org/208020/diff/1001/2018#newcode55
Line 55: /* color: $TEXT_COLOR$; */
Can this be removed? Or should it be put back?

[Finnur, 2009-09-18 01:09:36]

Had some comments, but looks like aa beat me to it. :)
LGTM for the rest. 

On 2009/09/18 00:44:37, Aaron Boodman wrote:
> http://codereview.chromium.org/208020/diff/1001/2002
> File chrome/browser/extensions/extension_host.cc (right):
> 
> http://codereview.chromium.org/208020/diff/1001/2002#newcode210
> Line 210: render_view_host()->InsertCSSInWebFrame(L"", css,
> "ToolstripThemeCss");
> Nit: can we change this string to have "CSS" capitalized?
> 
> http://codereview.chromium.org/208020/diff/1001/2003
> File chrome/browser/extensions/extension_host.h (right):
> 
> http://codereview.chromium.org/208020/diff/1001/2003#newcode72
> Line 72: //
> You just blew my mind, dude.
> 
> http://codereview.chromium.org/208020/diff/1001/2012
> File chrome/renderer/extensions/extension_process_bindings.cc (right):
> 
> http://codereview.chromium.org/208020/diff/1001/2012#newcode164
> Line 164: (*it)->view_type() == ViewType::EXTENSION_MOLE)) {
> Suggest reworking this logic to make easier to read. Maybe pull out a short
> helper function that can peel off each case and return early?
> 
> http://codereview.chromium.org/208020/diff/1001/2017
> File chrome/renderer/resources/extension_process_bindings.js (right):
> 
> http://codereview.chromium.org/208020/diff/1001/2017#newcode119
> Line 119: var modeClass = "extension_" + type;
> It is more common in CSS to use dash as the word separator.
> 
> http://codereview.chromium.org/208020/diff/1001/2017#newcode124
> Line 124: classes.forEach(function(cls) {
> Heh, if we have .forEach built-in, why are we defining it up above? Can you
> convert the usages of that function to the built-in and remove it as long as
> you're in here?
> 
> http://codereview.chromium.org/208020/diff/1001/2018
> File chrome/renderer/resources/extensions_toolstrip.css (right):
> 
> http://codereview.chromium.org/208020/diff/1001/2018#newcode15
> Line 15: /*color: $TEXT_COLOR$; */
> Looks like this can just be removed?
> 
> http://codereview.chromium.org/208020/diff/1001/2018#newcode55
> Line 55: /* color: $TEXT_COLOR$; */
> Can this be removed? Or should it be put back?

[Erik does not do reviews, 2009-09-18 01:29:21]

> lots of other good comments

Fixed.

http://codereview.chromium.org/208020/diff/1001/2012
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/208020/diff/1001/2012#newcode164
Line 164: (*it)->view_type() == ViewType::EXTENSION_MOLE)) {
On 2009/09/18 00:44:37, Aaron Boodman wrote:
> Suggest reworking this logic to make easier to read. Maybe pull out a short
> helper function that can peel off each case and return early?

great idea.  it looks much better this way.

http://codereview.chromium.org/208020/diff/1001/2017
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/208020/diff/1001/2017#newcode119
Line 119: var modeClass = "extension_" + type;
On 2009/09/18 00:44:37, Aaron Boodman wrote:
> It is more common in CSS to use dash as the word separator.

good point.  Also, with a second look, I decided to use "chrome-" instead of
"extension-".

[Aaron Boodman, 2009-09-18 18:24:53]

Nice.

LGTM