Implement about:labs

Implement about:labs

Tabpose is currently the only lab on mac, tabs-on-left the only lab on windows. Nothing for linux yet.

BUG=53399
TEST=Go to about:labs. Should have one feature on windows and osx each, none on linux yet. about:labs should not be visible on the stable channel. Labs that were enabled on the dev channel should not be enabled on the stable channel. about:labs in chromeos should still work (they use a different implementation)

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

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

[viettrungluu, 2010-08-26 23:48:35]

Haven't looked at it all yet. Could I get a screenshot?

http://codereview.chromium.org/3152055/diff/25002/27005
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27005#newcode258
chrome/browser/browser_about_handler.cc:258: if (kAllAboutPaths[i] == kLabsPath
||
Could you alphabetize these, or something?

http://codereview.chromium.org/3152055/diff/25002/27005#newcode1135
chrome/browser/browser_about_handler.cc:1135: #if !defined(OS_CHROMEOS)
What I said elsewhere about the #ifndef. Let's avoid them whenever possible.

http://codereview.chromium.org/3152055/diff/25002/27008
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27008#newcode122
chrome/browser/dom_ui/dom_ui_factory.cc:122: if (Labs::IsEnabled() && url.host()
== chrome::kChromeUILabsHost)
I suggest you just make Labs::IsEnabled() return false when OS_CHROMEOS is
defined, thereby avoiding the #ifndef ugliness.

http://codereview.chromium.org/3152055/diff/25002/27008#newcode238
chrome/browser/dom_ui/dom_ui_factory.cc:238: #if !defined(OS_CHROMEOS)
I say leave it in even for Chrome OS. (Or have the method return NULL.)

http://codereview.chromium.org/3152055/diff/25002/27014
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27014#newcode119
chrome/browser/prefs/browser_prefs.cc:119: #if !defined(OS_CHROMEOS)  // FIXME:
not on stable either. also elsewhere.
Wha?

http://codereview.chromium.org/3152055/diff/25002/27016
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3152055/diff/25002/27016#newcode1973
chrome/chrome_browser.gypi:1973: 'browser/lanbs.h',
huh?

[Nico, 2010-08-27 00:06:40]

thanks, sent you screenshots via mail

http://codereview.chromium.org/3152055/diff/25002/27005
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27005#newcode258
chrome/browser/browser_about_handler.cc:258: if (kAllAboutPaths[i] == kLabsPath
||
On 2010/08/26 23:48:35, viettrungluu wrote:
> Could you alphabetize these, or something?

Done.

http://codereview.chromium.org/3152055/diff/25002/27005#newcode1135
chrome/browser/browser_about_handler.cc:1135: #if !defined(OS_CHROMEOS)
On 2010/08/26 23:48:35, viettrungluu wrote:
> What I said elsewhere about the #ifndef. Let's avoid them whenever possible.

Done.

http://codereview.chromium.org/3152055/diff/25002/27008
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27008#newcode122
chrome/browser/dom_ui/dom_ui_factory.cc:122: if (Labs::IsEnabled() && url.host()
== chrome::kChromeUILabsHost)
On 2010/08/26 23:48:35, viettrungluu wrote:
> I suggest you just make Labs::IsEnabled() return false when OS_CHROMEOS is
> defined, thereby avoiding the #ifndef ugliness.

That's a good idea. Done

http://codereview.chromium.org/3152055/diff/25002/27008#newcode238
chrome/browser/dom_ui/dom_ui_factory.cc:238: #if !defined(OS_CHROMEOS)
On 2010/08/26 23:48:35, viettrungluu wrote:
> I say leave it in even for Chrome OS. (Or have the method return NULL.)

Done.

http://codereview.chromium.org/3152055/diff/25002/27014
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/3152055/diff/25002/27014#newcode119
chrome/browser/prefs/browser_prefs.cc:119: #if !defined(OS_CHROMEOS)  // FIXME:
not on stable either. also elsewhere.
On 2010/08/26 23:48:35, viettrungluu wrote:
> Wha?

Done.

http://codereview.chromium.org/3152055/diff/25002/27016
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3152055/diff/25002/27016#newcode1973
chrome/chrome_browser.gypi:1973: 'browser/lanbs.h',
On 2010/08/26 23:48:35, viettrungluu wrote:
> huh?

:-D Looks like getting the h files correct isn't all that important.

Done

[viettrungluu, 2010-08-27 02:52:16]

Have you checked if about:labs lays out correctly in RTL languages?

+arv to look over your HTML. LGTM with nits for everything else.

http://codereview.chromium.org/3152055/diff/34001/35013
File chrome/browser/labs.h (right):

http://codereview.chromium.org/3152055/diff/34001/35013#newcode15
chrome/browser/labs.h:15: namespace Labs {
I don't think we ever capitalize namespace names, do we?

http://codereview.chromium.org/3152055/diff/38002/39011
File chrome/browser/labs.cc (right):

http://codereview.chromium.org/3152055/diff/38002/39011#newcode24
chrome/browser/labs.cc:24: enum { kOsMac = 1, kOsWin = 2, kOsLinux = 4 };
Can you write these as 1 << 0, 1 << 1, 1 << 2 to make it more obvious that it's
a bitmask?

http://codereview.chromium.org/3152055/diff/38002/39011#newcode40
chrome/browser/labs.cc:40: int supported_platforms;  // bitmask
Somehow, I think bitmasks should be unsigned.

http://codereview.chromium.org/3152055/diff/38002/39014
File chrome/browser/resources/labs.html (right):

http://codereview.chromium.org/3152055/diff/38002/39014#newcode270
chrome/browser/resources/labs.html:270: <span class="experiment-name" dir="ltr"
The 'dir="ltr"' bits are inappropriate if we ever expect the names and
descriptions to be localized.

[Nico, 2010-08-27 03:21:05]

Thanks.

Arv is on vacation I believe, so I won't wait for him.

Haven't checked rtl yet due to lack of Windows at home. Will do so tomorrow.

http://codereview.chromium.org/3152055/diff/34001/35013
File chrome/browser/labs.h (right):

http://codereview.chromium.org/3152055/diff/34001/35013#newcode15
chrome/browser/labs.h:15: namespace Labs {
On 2010/08/27 02:52:16, viettrungluu wrote:
> I don't think we ever capitalize namespace names, do we?

I guess not.

I'm always unsure if we prefer classes with static methods or namespaces for
things like this. Namespaces seem superior, but lots of places have classes, so
this was my unconscious attempt at a compromise :-P

done

http://codereview.chromium.org/3152055/diff/38002/39011
File chrome/browser/labs.cc (right):

http://codereview.chromium.org/3152055/diff/38002/39011#newcode24
chrome/browser/labs.cc:24: enum { kOsMac = 1, kOsWin = 2, kOsLinux = 4 };
On 2010/08/27 02:52:17, viettrungluu wrote:
> Can you write these as 1 << 0, 1 << 1, 1 << 2 to make it more obvious that
it's
> a bitmask?

I tried, and I could. Done.

http://codereview.chromium.org/3152055/diff/38002/39011#newcode40
chrome/browser/labs.cc:40: int supported_platforms;  // bitmask
On 2010/08/27 02:52:17, viettrungluu wrote:
> Somehow, I think bitmasks should be unsigned.

So that we can support 32 OSs instead of a mere 31? :-)

Done

http://codereview.chromium.org/3152055/diff/38002/39014
File chrome/browser/resources/labs.html (right):

http://codereview.chromium.org/3152055/diff/38002/39014#newcode270
chrome/browser/resources/labs.html:270: <span class="experiment-name" dir="ltr"
On 2010/08/27 02:52:17, viettrungluu wrote:
> The 'dir="ltr"' bits are inappropriate if we ever expect the names and
> descriptions to be localized.

Taken from about:plugins…

[viettrungluu, 2010-08-27 03:27:32]

On 2010/08/27 03:21:05, Nico wrote:
> Thanks.
> 
> Arv is on vacation I believe, so I won't wait for him.
> 
> Haven't checked rtl yet due to lack of Windows at home. Will do so tomorrow.

You can just switch your language to an RTL one, restart Chromium, and observe
the page (even on a Mac). It's not like you're actually going to see RTL text,
but you can check that the layout is right.

[viettrungluu, 2010-08-27 03:28:45]

Argh. "Reply" doesn't send drafts. I also forgot to add the all-important "ship
it" (a.k.a. LGTM) once you've checked that it looks okay in RTL.

I have one more thing to add, but I don't feel like cutting-and-pasting this
message.

http://codereview.chromium.org/3152055/diff/38002/39014
File chrome/browser/resources/labs.html (right):

http://codereview.chromium.org/3152055/diff/38002/39014#newcode270
chrome/browser/resources/labs.html:270: <span class="experiment-name" dir="ltr"
On 2010/08/27 03:21:06, Nico wrote:
> On 2010/08/27 02:52:17, viettrungluu wrote:
> > The 'dir="ltr"' bits are inappropriate if we ever expect the names and
> > descriptions to be localized.
> 
> Taken from about:plugins…

I know; I wrote that. But the plugin descriptions are never localized (since
there's simply no mechanism for them to be).

(Of course, it'd do the wrong thing if a plugin ever supplied a description in
an RTL language, but there's not a whole lot that can be done about that within
reason.)

[viettrungluu, 2010-08-27 03:31:14]

http://codereview.chromium.org/3152055/diff/34001/35013
File chrome/browser/labs.h (right):

http://codereview.chromium.org/3152055/diff/34001/35013#newcode15
chrome/browser/labs.h:15: namespace Labs {
On 2010/08/27 03:21:06, Nico wrote:
> On 2010/08/27 02:52:16, viettrungluu wrote:
> > I don't think we ever capitalize namespace names, do we?
> 
> I guess not.
> 
> I'm always unsure if we prefer classes with static methods or namespaces for
> things like this. Namespaces seem superior, but lots of places have classes,
so
> this was my unconscious attempt at a compromise :-P
> 
> done

Yeah, that occurred to me later. I'm torn, since lowercase namespaces are
generally reserved for pretty general things. (And it's moderately silly to have
a class with only static methods. Maybe I would have liked it more if it had a
longer name, like LabsExperiments. <shrug>)