Add a new App launcher promo to the apps page / NTP.

Add a new App launcher promo to the apps page / NTP.

Simple change in chrome/browser/chrome_browser_main.cc
TBR=ben@chromium.org
BUG=180475
TEST=Make sure the apps promo only show on apps page of the NTP (or chrome://apps) and that it links to webstore (specific page to be added later on the webstore) and that it doesn't come back on a given installation of Chrome once dismissed with the X close button. That dimiss state can be reset with the command line switch --reset-show-apps-promo to help test it. Also, this shouldn't show when the App Launcher is installer... since no apps page should be shown anyway (tested with --show-app-list-shortcut)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190772

[MAD, 2013-03-21 15:06:09]

Can you do an initial CR before I ask dbeam@ for OWNERs approval?

Thanks!

BYE
MAD...

[MAD, 2013-03-21 15:11:52]

Adding benwells@ for the apps stuff...

[Dan Beam, 2013-03-21 15:45:14]

why is this not using the promo infrastructure and just setting some arbitrary
data in its payload?

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: border-width: 1px;
border: 3px solid lightgray;
border-radius: 2px;
border-top-width: 2px;

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:130: font-weight: normal;
font: 16px normal Arial, Helvetica, sans-serif;

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:138: -webkit-box-shadow: rgba(0, 0,
0, 0.0980392) 0 1px 1px 0;
^ remove

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:160: border-top-width: 1px;
border: 1px solid rgb(47, 91, 183);

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:174: margin-top: 0;
margin: 0;

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:181: padding-top: 0;
padding: 0 8px;

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:194: border-color: rgb(47, 91, 183);
border: 1px solid rgb(47, 91, 183);

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:482: * @param {bool} show
Identifies if we should show or hide the promo.
{boolean}

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:486:
loadTimeData.overrideValues({'showAppLauncherPromo': show});
nit: loadTimeData.overrideValues({showAppLauncherPromo: show});

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:511: this.shownPage =
loadTimeData.getInteger('apps_page_id');
^ er, shouldn't you be selectCardByValue()'ing here?

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:678: this.shownPage !=
loadTimeData.getInteger('apps_page_id');
please combine this and the same code above into a function and call from both
places

[MAD, 2013-03-21 16:47:46]

For the question about using the promo server, I will let benwells@ (and maybe
rsesek@?) answer. I think they explored that possibility... I didn't... Should
I?

How about these fixes for the rest?

Thanks!

BYE
MAD

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: border-width: 1px;
On 2013/03/21 15:45:14, Dan Beam wrote:
> border: 3px solid lightgray;
> border-radius: 2px;
> border-top-width: 2px;

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:130: font-weight: normal;
On 2013/03/21 15:45:14, Dan Beam wrote:
> font: 16px normal Arial, Helvetica, sans-serif;

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:138: -webkit-box-shadow: rgba(0, 0,
0, 0.0980392) 0 1px 1px 0;
On 2013/03/21 15:45:14, Dan Beam wrote:
> ^ remove

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:160: border-top-width: 1px;
On 2013/03/21 15:45:14, Dan Beam wrote:
> border: 1px solid rgb(47, 91, 183);

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:174: margin-top: 0;
On 2013/03/21 15:45:14, Dan Beam wrote:
> margin: 0;

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:181: padding-top: 0;
On 2013/03/21 15:45:14, Dan Beam wrote:
> padding: 0 8px;

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:194: border-color: rgb(47, 91, 183);
On 2013/03/21 15:45:14, Dan Beam wrote:
> border: 1px solid rgb(47, 91, 183);

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:482: * @param {bool} show
Identifies if we should show or hide the promo.
On 2013/03/21 15:45:14, Dan Beam wrote:
> {boolean}

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:486:
loadTimeData.overrideValues({'showAppLauncherPromo': show});
On 2013/03/21 15:45:14, Dan Beam wrote:
> nit: loadTimeData.overrideValues({showAppLauncherPromo: show});

Done.

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:511: this.shownPage =
loadTimeData.getInteger('apps_page_id');
On 2013/03/21 15:45:14, Dan Beam wrote:
> ^ er, shouldn't you be selectCardByValue()'ing here?

Yeah, I wasn't sure... Things seems to work anyway, so I tried to make a minimal
change for what wasn't working, i.e., the shownPage state which I need to decide
whether I show the promo box... Now that you asked for it, I changed it, and it
still works... So... fingers crossed... :-)

https://codereview.chromium.org/12985002/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:678: this.shownPage !=
loadTimeData.getInteger('apps_page_id');
On 2013/03/21 15:45:14, Dan Beam wrote:
> please combine this and the same code above into a function and call from both
> places

Done.
And it made me realize that if we would change the Pref back to true (which
currently can't happen, but might eventually), then we would have been able to
show the promo on non-Apps pages... Good catch! :-)

[Robert Sesek, 2013-03-21 17:49:21]

On 2013/03/21 16:47:46, MAD wrote:
> For the question about using the promo server, I will let benwells@ (and maybe
> rsesek@?) answer. I think they explored that possibility... I didn't... Should
> I?

It may be beneficial to do so, because it'd allow us to specify the strings and
URL(s) on the server, which would enable us to fix/change them after branch
point.

[gideonwald, 2013-03-21 17:58:08]

On 2013/03/21 17:49:21, rsesek wrote:
> On 2013/03/21 16:47:46, MAD wrote:
> > For the question about using the promo server, I will let benwells@ (and
maybe
> > rsesek@?) answer. I think they explored that possibility... I didn't...
Should
> > I?
> 
> It may be beneficial to do so, because it'd allow us to specify the strings
and
> URL(s) on the server, which would enable us to fix/change them after branch
> point.

We're worried that the promo server UI is pretty minimal and easy to
ignore/dismiss. We want to be promoting the app launcher fairly aggressively
here, because the current Apps experience in 1993 is unfortunate, and the app
launcher team wants as substantial of a population of users opting in as
possible so that they can get feedback and data.

[Robert Sesek, 2013-03-21 17:59:19]

On 2013/03/21 17:58:08, gideonwald wrote:
> On 2013/03/21 17:49:21, rsesek wrote:
> > On 2013/03/21 16:47:46, MAD wrote:
> > > For the question about using the promo server, I will let benwells@ (and
> maybe
> > > rsesek@?) answer. I think they explored that possibility... I didn't...
> Should
> > > I?
> > 
> > It may be beneficial to do so, because it'd allow us to specify the strings
> and
> > URL(s) on the server, which would enable us to fix/change them after branch
> > point.
> 
> We're worried that the promo server UI is pretty minimal and easy to
> ignore/dismiss. We want to be promoting the app launcher fairly aggressively
> here, because the current Apps experience in 1993 is unfortunate, and the app
> launcher team wants as substantial of a population of users opting in as
> possible so that they can get feedback and data.

I was suggesting to use the promo server as a data feed for this UI, not to use
the text UI.

[guohui, 2013-03-21 18:06:06]

On 2013/03/21 17:59:19, rsesek wrote:
> On 2013/03/21 17:58:08, gideonwald wrote:
> > On 2013/03/21 17:49:21, rsesek wrote:
> > > On 2013/03/21 16:47:46, MAD wrote:
> > > > For the question about using the promo server, I will let benwells@ (and
> > maybe
> > > > rsesek@?) answer. I think they explored that possibility... I didn't...
> > Should
> > > > I?
> > > 
> > > It may be beneficial to do so, because it'd allow us to specify the
strings
> > and
> > > URL(s) on the server, which would enable us to fix/change them after
branch
> > > point.
> > 
> > We're worried that the promo server UI is pretty minimal and easy to
> > ignore/dismiss. We want to be promoting the app launcher fairly aggressively
> > here, because the current Apps experience in 1993 is unfortunate, and the
app
> > launcher team wants as substantial of a population of users opting in as
> > possible so that they can get feedback and data.
> 
> I was suggesting to use the promo server as a data feed for this UI, not to
use
> the text UI.
+1 if possible. This will allow us to update the promo text including the
webstore URL without a Chrome code change.

[Dan Beam, 2013-03-21 18:33:21]

On 2013/03/21 17:58:08, gideonwald wrote:
> On 2013/03/21 17:49:21, rsesek wrote:
> > On 2013/03/21 16:47:46, MAD wrote:
> > > For the question about using the promo server, I will let benwells@ (and
> maybe
> > > rsesek@?) answer. I think they explored that possibility... I didn't...
> Should
> > > I?
> > 
> > It may be beneficial to do so, because it'd allow us to specify the strings
> and
> > URL(s) on the server, which would enable us to fix/change them after branch
> > point.
> 
> We're worried that the promo server UI is pretty minimal and easy to
> ignore/dismiss. We want to be promoting the app launcher fairly aggressively
> here, because the current Apps experience in 1993 is unfortunate, and the app
> launcher team wants as substantial of a population of users opting in as
> possible so that they can get feedback and data.

If you don't care about opting in, FieldTrial instead?

[MAD, 2013-03-21 18:41:16]

I don't mind changing the code to get the strings and URL from the promo server,
I've just never done it, can you point me to examples I could inspire from?

[Robert Sesek, 2013-03-21 18:52:52]

On 2013/03/21 18:41:16, MAD wrote:
> I don't mind changing the code to get the strings and URL from the promo
server,
> I've just never done it, can you point me to examples I could inspire from?

Promos are fetched using the PromoResourceService:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web...

I think the right thing to do would be to add a new PromoType and have this be
the handler for it.

[MAD, 2013-03-21 19:11:18]

I have no experience with this... Do you think this is feasible for M27?

I'll start playing with this stuff in a separate local branch, in case we
decide to go with what I currently have for M27...

Gideon/Ben, would you like to use Finch to control how many people see the
promo if we don't use the promo server?



On Thu, Mar 21, 2013 at 2:52 PM, <rsesek@chromium.org> wrote:

> On 2013/03/21 18:41:16, MAD wrote:
>
>> I don't mind changing the code to get the strings and URL from the promo
>>
> server,
>
>> I've just never done it, can you point me to examples I could inspire
>> from?
>>
>
> Promos are fetched using the PromoResourceService:
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/web_**resource/promo_resource_**
>
service.cc&q=promo_resource_**service&sq=package:chromium<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web_resource/promo_resource_service.cc&q=promo_resource_service&sq=package:chromium>
>
> I think the right thing to do would be to add a new PromoType and have
> this be
> the handler for it.
>
>
https://codereview.chromium.**org/12985002/<https://codereview.chromium.org/1...
>



-- 
*
BYE
MAD
*

[MAD, 2013-03-21 19:14:12]

Ho! And wouldn't this interfere with a text base app launcher promo? I
guess we would need two new types? One for text and one for the overlay box?


On Thu, Mar 21, 2013 at 3:10 PM, Marc-Andre Decoste <mad@chromium.org>wrote:

> I have no experience with this... Do you think this is feasible for M27?
>
> I'll start playing with this stuff in a separate local branch, in case we
> decide to go with what I currently have for M27...
>
> Gideon/Ben, would you like to use Finch to control how many people see the
> promo if we don't use the promo server?
>
>
>
> On Thu, Mar 21, 2013 at 2:52 PM, <rsesek@chromium.org> wrote:
>
>> On 2013/03/21 18:41:16, MAD wrote:
>>
>>> I don't mind changing the code to get the strings and URL from the promo
>>>
>> server,
>>
>>> I've just never done it, can you point me to examples I could inspire
>>> from?
>>>
>>
>> Promos are fetched using the PromoResourceService:
>> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/chrome/browser/web_**resource/promo_resource_**
>>
service.cc&q=promo_resource_**service&sq=package:chromium<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web_resource/promo_resource_service.cc&q=promo_resource_service&sq=package:chromium>
>>
>> I think the right thing to do would be to add a new PromoType and have
>> this be
>> the handler for it.
>>
>>
https://codereview.chromium.**org/12985002/<https://codereview.chromium.org/1...
>>
>
>
>
> --
> *
> BYE
> MAD
> *
>



-- 
*
BYE
MAD
*

[Robert Sesek, 2013-03-21 20:19:06]

The text promo already has a type, so it wouldn't interfere (though they could
run at the same time if configured to do so).

This would probably just entail adding another type, and then reading it in the
C++ controller for this class using NotificationPromo::InitFromPrefs(). I'm not
too familiar with the client code, though. dbeam@ and achuith@ did most of the
work.

On 2013/03/21 19:14:12, MAD wrote:
> Ho! And wouldn't this interfere with a text base app launcher promo? I
> guess we would need two new types? One for text and one for the overlay box?
> 
> 
> On Thu, Mar 21, 2013 at 3:10 PM, Marc-Andre Decoste <mad@chromium.org>wrote:
> 
> > I have no experience with this... Do you think this is feasible for M27?
> >
> > I'll start playing with this stuff in a separate local branch, in case we
> > decide to go with what I currently have for M27...
> >
> > Gideon/Ben, would you like to use Finch to control how many people see the
> > promo if we don't use the promo server?
> >
> >
> >
> > On Thu, Mar 21, 2013 at 2:52 PM, <mailto:rsesek@chromium.org> wrote:
> >
> >> On 2013/03/21 18:41:16, MAD wrote:
> >>
> >>> I don't mind changing the code to get the strings and URL from the promo
> >>>
> >> server,
> >>
> >>> I've just never done it, can you point me to examples I could inspire
> >>> from?
> >>>
> >>
> >> Promos are fetched using the PromoResourceService:
> >> https://code.google.com/p/**chromium/codesearch#chromium/**
> >> src/chrome/browser/web_**resource/promo_resource_**
> >>
>
service.cc&q=promo_resource_**service&sq=package:chromium<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web_resource/promo_resource_service.cc&q=promo_resource_service&sq=package:chromium>
> >>
> >> I think the right thing to do would be to add a new PromoType and have
> >> this be
> >> the handler for it.
> >>
> >>
>
https://codereview.chromium.**org/12985002/%3Chttps://codereview.chromium.org...>
> >>
> >
> >
> >
> > --
> > *
> > BYE
> > MAD
> > *
> >
> 
> 
> 
> -- 
> *
> BYE
> MAD
> *

[MAD, 2013-03-21 20:29:00]

Yeah, I'm toying with that right now, using the promo_text for the text
shown in the promo box, above the learn more button, and use a specific
payload for the webstore URL...

The one thing I wonder is how I can test this? Is there an easier way to do
this than using a test server?

---------- Forwarded message ----------
From: <rsesek@chromium.org>
Date: Thu, Mar 21, 2013 at 4:19 PM
Subject: Re: Add a new App launcher promo to the apps page / NTP. (issue
12985002)
To: mad@chromium.org, guohui@chromium.org, benwells@chromium.org
Cc: chromium-reviews@chromium.org, dbeam+watch-ntp@chromium.org,
tfarina@chromium.org, arv+watch@chromium.org,
chromium-apps-reviews@chromium.org, estade+watch@chromium.org,
pedrosimonetti+watch@chromium.org


The text promo already has a type, so it wouldn't interfere (though they
could
run at the same time if configured to do so).

This would probably just entail adding another type, and then reading it in
the
C++ controller for this class using NotificationPromo::**InitFromPrefs().
I'm not
too familiar with the client code, though. dbeam@ and achuith@ did most of
the
work.


On 2013/03/21 19:14:12, MAD wrote:

> Ho! And wouldn't this interfere with a text base app launcher promo? I
> guess we would need two new types? One for text and one for the overlay
> box?
>


 On Thu, Mar 21, 2013 at 3:10 PM, Marc-Andre Decoste <mad@chromium.org
> >wrote:
>

 > I have no experience with this... Do you think this is feasible for M27?
> >
> > I'll start playing with this stuff in a separate local branch, in case we
> > decide to go with what I currently have for M27...
> >
> > Gideon/Ben, would you like to use Finch to control how many people see
> the
> > promo if we don't use the promo server?
> >
> >
> >
> > On Thu, Mar 21, 2013 at 2:52 PM, <mailto:rsesek@chromium.org> wrote:
> >
> >> On 2013/03/21 18:41:16, MAD wrote:
> >>
> >>> I don't mind changing the code to get the strings and URL from the
> promo
> >>>
> >> server,
> >>
> >>> I've just never done it, can you point me to examples I could inspire
> >>> from?
> >>>
> >>
> >> Promos are fetched using the PromoResourceService:
> >>
https://code.google.com/p/****chromium/codesearch#chromium/****<https://code....
> >> src/chrome/browser/web_****resource/promo_resource_**
> >>
>

service.cc&q=promo_resource_****service&sq=package:chromium<ht**
tps://code.google.com/p/**chromium/codesearch#chromium/**
src/chrome/browser/web_**resource/promo_resource_**
service.cc&q=promo_resource_**service&sq=package:chromium<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web_resource/promo_resource_service.cc&q=promo_resource_service&sq=package:chromium>
>

 >>
> >> I think the right thing to do would be to add a new PromoType and have
> >> this be
> >> the handler for it.
> >>
> >>
>

https://codereview.chromium.****org/12985002/%3Chttps://codere**
view.chromium.org/12985002/ <http://codereview.chromium.org/12985002/>>

> >>
> >
> >
> >
> > --
> > *
> > BYE
> > MAD
> > *
> >
>



 --
> *
> BYE
> MAD
> *
>



https://codereview.chromium.**org/12985002/<https://codereview.chromium.org/1...

[Robert Sesek, 2013-03-21 20:43:11]

You could just craft the JSON in the right format and use a simple local
HTTP server as the --promo-sever-url.

rsesek / @chromium.org

On Thu, Mar 21, 2013 at 4:28 PM, Marc-Andre Decoste <mad@chromium.org>wrote:

> Yeah, I'm toying with that right now, using the promo_text for the text
> shown in the promo box, above the learn more button, and use a specific
> payload for the webstore URL...
>
> The one thing I wonder is how I can test this? Is there an easier way to
> do this than using a test server?
>
>
> ---------- Forwarded message ----------
> From: <rsesek@chromium.org>
> Date: Thu, Mar 21, 2013 at 4:19 PM
> Subject: Re: Add a new App launcher promo to the apps page / NTP. (issue
> 12985002)
> To: mad@chromium.org, guohui@chromium.org, benwells@chromium.org
> Cc: chromium-reviews@chromium.org, dbeam+watch-ntp@chromium.org,
> tfarina@chromium.org, arv+watch@chromium.org,
> chromium-apps-reviews@chromium.org, estade+watch@chromium.org,
> pedrosimonetti+watch@chromium.org
>
>
> The text promo already has a type, so it wouldn't interfere (though they
> could
> run at the same time if configured to do so).
>
> This would probably just entail adding another type, and then reading it
> in the
> C++ controller for this class using NotificationPromo::**InitFromPrefs().
> I'm not
> too familiar with the client code, though. dbeam@ and achuith@ did most
> of the
> work.
>
>
> On 2013/03/21 19:14:12, MAD wrote:
>
>> Ho! And wouldn't this interfere with a text base app launcher promo? I
>> guess we would need two new types? One for text and one for the overlay
>> box?
>>
>
>
>  On Thu, Mar 21, 2013 at 3:10 PM, Marc-Andre Decoste <mad@chromium.org
>> >wrote:
>>
>
>  > I have no experience with this... Do you think this is feasible for M27?
>> >
>> > I'll start playing with this stuff in a separate local branch, in case
>> we
>> > decide to go with what I currently have for M27...
>> >
>> > Gideon/Ben, would you like to use Finch to control how many people see
>> the
>> > promo if we don't use the promo server?
>> >
>> >
>> >
>> > On Thu, Mar 21, 2013 at 2:52 PM, <mailto:rsesek@chromium.org> wrote:
>> >
>> >> On 2013/03/21 18:41:16, MAD wrote:
>> >>
>> >>> I don't mind changing the code to get the strings and URL from the
>> promo
>> >>>
>> >> server,
>> >>
>> >>> I've just never done it, can you point me to examples I could inspire
>> >>> from?
>> >>>
>> >>
>> >> Promos are fetched using the PromoResourceService:
>> >>
https://code.google.com/p/****chromium/codesearch#chromium/****<https://code....
>> >> src/chrome/browser/web_****resource/promo_resource_**
>> >>
>>
>
> service.cc&q=promo_resource_****service&sq=package:chromium<ht**
> tps://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/web_**resource/promo_resource_**
>
service.cc&q=promo_resource_**service&sq=package:chromium<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/web_resource/promo_resource_service.cc&q=promo_resource_service&sq=package:chromium>
> >
>
>  >>
>> >> I think the right thing to do would be to add a new PromoType and have
>> >> this be
>> >> the handler for it.
>> >>
>> >>
>>
>
> https://codereview.chromium.****org/12985002/%3Chttps://codere**
> view.chromium.org/12985002/ <http://codereview.chromium.org/12985002/>>
>
>> >>
>> >
>> >
>> >
>> > --
>> > *
>> > BYE
>> > MAD
>> > *
>> >
>>
>
>
>
>  --
>> *
>> BYE
>> MAD
>> *
>>
>
>
>
>
https://codereview.chromium.**org/12985002/<https://codereview.chromium.org/1...
>
>

[MAD, 2013-03-21 23:09:39]

OK, I added a Field Trial to control the showing of the promo and removed the
command switch to test it, since we can do this with a field trial now.

Please take another look...

Thanks!

BYE
MAD

[benwells, 2013-03-21 23:22:56]

src/apps lgtm

Thanks MAD!

[MAD, 2013-03-22 00:40:14]

Adding ASvitkine@ for the field trial initialization changes. Feel free to look
at the other files too if you feel like it... :-)

Thanks!

BYE
MAD

[MAD, 2013-03-22 15:44:39]

Ping Dan? Can you continue the review?

Hui? Do you have any comment?

Alexei? Should I ask Steve for the field trial related changes?

Thanks all!

BYE
MAD...

[guohui, 2013-03-22 16:47:56]

On 2013/03/22 15:44:39, MAD wrote:
> Ping Dan? Can you continue the review?
> 
> Hui? Do you have any comment?
> 
> Alexei? Should I ask Steve for the field trial related changes?
> 
> Thanks all!
> 
> BYE
> MAD...

lgtm, thanks!

[asvitkine_google, 2013-03-22 16:54:07]

Looking at it now.


On Fri, Mar 22, 2013 at 9:47 AM, <guohui@chromium.org> wrote:

> On 2013/03/22 15:44:39, MAD wrote:
>
>> Ping Dan? Can you continue the review?
>>
>
>  Hui? Do you have any comment?
>>
>
>  Alexei? Should I ask Steve for the field trial related changes?
>>
>
>  Thanks all!
>>
>
>  BYE
>> MAD...
>>
>
> lgtm, thanks!
>
>
https://codereview.chromium.**org/12985002/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-03-22 16:57:35]

https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc
File apps/field_trial_names.cc (right):

https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc...
apps/field_trial_names.cc:14: const char kResetShowLauncherPromoPrefGroup[] =
"ResetShowPromoPref";
Nit: Change suffix to "*GroupName" to be consistent with the other variable
above.

https://codereview.chromium.org/12985002/diff/67005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12985002/diff/67005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:74: if
(base::FieldTrialList::FindFullName(apps::kLauncherPromoTrialName) ==
Please encapsulate this in a separate function and call it from
SetupDesktopFieldTrials().

[MAD, 2013-03-22 17:11:35]

Thanks! All done,

Let me know if it's OK...

BYE
MAD

https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc
File apps/field_trial_names.cc (right):

https://codereview.chromium.org/12985002/diff/67005/apps/field_trial_names.cc...
apps/field_trial_names.cc:14: const char kResetShowLauncherPromoPrefGroup[] =
"ResetShowPromoPref";
On 2013/03/22 16:57:35, Alexei Svitkine wrote:
> Nit: Change suffix to "*GroupName" to be consistent with the other variable
> above.

Done.

https://codereview.chromium.org/12985002/diff/67005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12985002/diff/67005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:74: if
(base::FieldTrialList::FindFullName(apps::kLauncherPromoTrialName) ==
On 2013/03/22 16:57:35, Alexei Svitkine wrote:
> Please encapsulate this in a separate function and call it from
> SetupDesktopFieldTrials().

Done.

[Alexei Svitkine (slow), 2013-03-22 17:14:58]

lgtm

[MAD, 2013-03-22 22:09:38]

Ping Dan? Will you have time for this before branch point?

Or should I find another reviewer/owner?

Thanks!

BYE
MAD...

[Dan Beam, 2013-03-22 22:45:45]

estade@/jhawkins@ are out, arv@ might be able to help

[MAD, 2013-03-25 14:10:35]

Thanks Dan!

arv@ will you have time to look at this today? Hoping to get this in before the
branch point...

BYE
MAD

[arv (Not doing code reviews), 2013-03-25 15:04:38]

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:126: font: 16px normal Arial,
Helvetica, sans-serif;
Why does the font need to be reset? It is usually better to just inherit the
font.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:137: box-shadow: rgba(0, 0, 0,
0.0980392) 0 1px 1px 0;
in case 0.0980391 is too dark?

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:144: margin: 0;
why is this needed?

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:145: outline-color: white;
Can't you do "outline: 0 none white"?

Which makes me wonder why you need anything but "outline: 0"?

If you are removing outline you need some other way to make :focused have a
distinct look.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:153: vertical-align: baseline;
baseline is the default, isn't it?

I don't understand why you need to specify all of these properties? Try opening
your page and the web inspector and disable one by one and see which ones are
actually needed.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:97: <div class="apps-promo-text"
id="apps-promo-text">
Why do you need div/div/span? Can one element be used instead of 3?

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:100: <a
href="https://chrome.google.com/webstore/launcher">
Same here?

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:105: <img
src="app_launcher_promo.png"/>
No /> in HTML

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:106: <div class="close-button"
id="app-launcher-promo-close-button"></div>
Should this be a <button>? What is the accessibility story here?

[MAD, 2013-03-25 15:32:23]

Thanks a lot... How about this?

BYE
MAD

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:126: font: 16px normal Arial,
Helvetica, sans-serif;
On 2013/03/25 15:04:38, arv wrote:
> Why does the font need to be reset? It is usually better to just inherit the
> font.

You mean set the font on the outer div and inherit it in the inner divs, right?
If so, done!

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:137: box-shadow: rgba(0, 0, 0,
0.0980392) 0 1px 1px 0;
On 2013/03/25 15:04:38, arv wrote:
> in case 0.0980391 is too dark?

I copied this from the internal style recommendation, I don't mind removing
it...

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:144: margin: 0;
On 2013/03/25 15:04:38, arv wrote:
> why is this needed?

Again, copied from internal styling... I don't mind removing...

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:145: outline-color: white;
On 2013/03/25 15:04:38, arv wrote:
> Can't you do "outline: 0 none white"?
> 
> Which makes me wonder why you need anything but "outline: 0"?
> 
> If you are removing outline you need some other way to make :focused have a
> distinct look.

Done.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:153: vertical-align: baseline;
On 2013/03/25 15:04:38, arv wrote:
> baseline is the default, isn't it?
> 
> I don't understand why you need to specify all of these properties? Try
opening
> your page and the web inspector and disable one by one and see which ones are
> actually needed.

Done.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:97: <div class="apps-promo-text"
id="apps-promo-text">
On 2013/03/25 15:04:38, arv wrote:
> Why do you need div/div/span? Can one element be used instead of 3?

I followed the patterns above...

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:100: <a
href="https://chrome.google.com/webstore/launcher">
On 2013/03/25 15:04:38, arv wrote:
> Same here?

Done.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:105: <img
src="app_launcher_promo.png"/>
On 2013/03/25 15:04:38, arv wrote:
> No /> in HTML

Done.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:106: <div class="close-button"
id="app-launcher-promo-close-button"></div>
On 2013/03/25 15:04:38, arv wrote:
> Should this be a <button>? What is the accessibility story here?

Everywhere else I saw the close-button, it was in a div.

[arv (Not doing code reviews), 2013-03-26 14:02:38]

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:126: font: 16px normal Arial,
Helvetica, sans-serif;
On 2013/03/25 15:32:24, MAD wrote:
> On 2013/03/25 15:04:38, arv wrote:
> > Why does the font need to be reset? It is usually better to just inherit the
> > font.
> 
> You mean set the font on the outer div and inherit it in the inner divs,
right?
> If so, done!

The font is already set on the body.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:137: box-shadow: rgba(0, 0, 0,
0.0980392) 0 1px 1px 0;
On 2013/03/25 15:32:24, MAD wrote:
> On 2013/03/25 15:04:38, arv wrote:
> > in case 0.0980391 is too dark?
> 
> I copied this from the internal style recommendation, I don't mind removing
> it...

I just thought it really strange that they it is not 0.098

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:144: margin: 0;
On 2013/03/25 15:32:24, MAD wrote:
> On 2013/03/25 15:04:38, arv wrote:
> > why is this needed?
> 
> Again, copied from internal styling... I don't mind removing...

The goal is to not have anything that is not needed here. It makes things harder
to maintain (as well as increases code size in general).

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo"
id="app-launcher-promo" hidden>
You should not have both id and class that have the same value. Classes are
usually used when you want to classify multiple elements and id is used to
identify a single element. In this case it seems like you can remove the class.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:97: <div class="apps-promo-text"
id="apps-promo-text">
On 2013/03/25 15:32:24, MAD wrote:
> On 2013/03/25 15:04:38, arv wrote:
> > Why do you need div/div/span? Can one element be used instead of 3?
> 
> I followed the patterns above... 

The app-launcher-promo is needed to do the centering. However, the inner span
can be removed I believe.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:106: <div class="close-button"
id="app-launcher-promo-close-button"></div>
On 2013/03/25 15:32:24, MAD wrote:
> On 2013/03/25 15:04:38, arv wrote:
> > Should this be a <button>? What is the accessibility story here?
> 
> Everywhere else I saw the close-button, it was in a div.

Really? Look at the #notification above:

<button class="close-button custom-appearance" class="custom-appearance">

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: font-family: Arial, Helvetica,
sans-serif;
Remove font-family

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:114: position: fixed;
I'm concerned about this one too. The notifications are not fixed so I don't
really think this should be either. Did the mocks show how to handle this when
the page scrolls?

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:127: font-family: inherit;
Is this needed?. inherit is the default. Maybe someone is overriding it... see
why it is bad to override when you don't need to ;-)

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:140: cursor: default;
If it is a link, why don't we want it to show the hand cursor?

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.html:101: <div
class="apps-promo-learn-more" id="apps-promo-learn-more"
This can be <a href="..." ... i18n-content="learn_more"></a>

Any time you have an extra wrapping element you should think why. There are some
useful cases (centering a block without using a flex box is one of the few valid
cases for example).

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.html:105: <img
src="app_launcher_promo.png"></img>
Invalid html. This should be:

<img src="app_launcher_promo.png">

[MAD, 2013-03-26 15:42:43]

OK, how about this now?

Some of your suggestions didn't work and I explained why in the comments
below...

Thanks again!

BYE
MAD

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo"
id="app-launcher-promo" hidden>
On 2013/03/26 14:02:39, arv wrote:
> You should not have both id and class that have the same value. Classes are
> usually used when you want to classify multiple elements and id is used to
> identify a single element. In this case it seems like you can remove the
class.

But I need the class for the CSS stuff... no?
And I need the ID to access it in JS and show/hide it.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:97: <div class="apps-promo-text"
id="apps-promo-text">
On 2013/03/26 14:02:39, arv wrote:
> On 2013/03/25 15:32:24, MAD wrote:
> > On 2013/03/25 15:04:38, arv wrote:
> > > Why do you need div/div/span? Can one element be used instead of 3?
> > 
> > I followed the patterns above... 
> 
> The app-launcher-promo is needed to do the centering. However, the inner span
> can be removed I believe.

Done.

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:106: <div class="close-button"
id="app-launcher-promo-close-button"></div>
On 2013/03/26 14:02:39, arv wrote:
> On 2013/03/25 15:32:24, MAD wrote:
> > On 2013/03/25 15:04:38, arv wrote:
> > > Should this be a <button>? What is the accessibility story here?
> > 
> > Everywhere else I saw the close-button, it was in a div.
> 
> Really? Look at the #notification above:
> 
> <button class="close-button custom-appearance" class="custom-appearance">

Ho, sorry, I didn't notice that one... :-/

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: font-family: Arial, Helvetica,
sans-serif;
On 2013/03/26 14:02:39, arv wrote:
> Remove font-family

If I do, I get the wrong font. 'Segoe UI', Tahoma, sans-serif

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:114: position: fixed;
Yes, the specs is to have this fixed above the footer...

I CC'd you on the thread and asked the question to make sure I didn't
misunderstand.

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:127: font-family: inherit;
On 2013/03/26 14:02:39, arv wrote:
> Is this needed?. inherit is the default. Maybe someone is overriding it... see
> why it is bad to override when you don't need to ;-)

Done.

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:140: cursor: default;
On 2013/03/26 14:02:39, arv wrote:
> If it is a link, why don't we want it to show the hand cursor?

Again, this is part of the internal spec I was following.

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.html:105: <img
src="app_launcher_promo.png"></img>
On 2013/03/26 14:02:39, arv wrote:
> Invalid html. This should be:
> 
> <img src="app_launcher_promo.png">

Done.

[arv (Not doing code reviews), 2013-03-26 15:53:34]

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo"
id="app-launcher-promo" hidden>
On 2013/03/26 15:42:43, MAD wrote:
> On 2013/03/26 14:02:39, arv wrote:
> > You should not have both id and class that have the same value. Classes are
> > usually used when you want to classify multiple elements and id is used to
> > identify a single element. In this case it seems like you can remove the
> class.
> 
> But I need the class for the CSS stuff... no?
> And I need the ID to access it in JS and show/hide it.

You can use an ID selector in the CSS.

#app-launcher-promo {
  ...
}

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: font-family: Arial, Helvetica,
sans-serif;
On 2013/03/26 15:42:43, MAD wrote:
> On 2013/03/26 14:02:39, arv wrote:
> > Remove font-family
> 
> If I do, I get the wrong font. 'Segoe UI', Tahoma, sans-serif

So, we don't want the promo to have the same font as everything else on that
page?

OK.

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:140: cursor: default;
On 2013/03/26 15:42:43, MAD wrote:
> On 2013/03/26 14:02:39, arv wrote:
> > If it is a link, why don't we want it to show the hand cursor?
> 
> Again, this is part of the internal spec I was following.

Done.

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.css:103: .app-launcher-promo {
What happened to the box-shadow? I was just pointing out that the alpha value
was really strange.

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:103:
id="app-launcher-promo-close-button">
<button> needs a closing tag.

[MAD, 2013-03-26 17:02:45]

Answered/Done...

Anything else?

Thanks again!

BYE
MAD

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/50023/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:96: <div class="app-launcher-promo"
id="app-launcher-promo" hidden>
On 2013/03/26 15:53:34, arv wrote:
> On 2013/03/26 15:42:43, MAD wrote:
> > On 2013/03/26 14:02:39, arv wrote:
> > > You should not have both id and class that have the same value. Classes
are
> > > usually used when you want to classify multiple elements and id is used to
> > > identify a single element. In this case it seems like you can remove the
> > class.
> > 
> > But I need the class for the CSS stuff... no?
> > And I need the ID to access it in JS and show/hide it.
> 
> You can use an ID selector in the CSS.
> 
> #app-launcher-promo {
>   ...
> }

Done.

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.css (right):

https://codereview.chromium.org/12985002/diff/9002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.css:110: font-family: Arial, Helvetica,
sans-serif;
I was just following the spec, I'll mention it on the spec thread...

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/12985002/diff/97001/chrome/browser/resources/...
chrome/browser/resources/ntp4/new_tab.html:103:
id="app-launcher-promo-close-button">
On 2013/03/26 15:53:34, arv wrote:
> <button> needs a closing tag.

Done.

[arv (Not doing code reviews), 2013-03-26 18:17:42]

LGTM

[commit-bot: I haz the power, 2013-03-26 18:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/12985002/91021

[commit-bot: I haz the power, 2013-03-26 18:33:11]

Presubmit check for 12985002-91021 failed and returned exit status 1.

INFO:root:Found 19 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/chrome_browser_main.cc

Presubmit checks took 3.2s to calculate.

[commit-bot: I haz the power, 2013-03-26 18:41:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/12985002/91021

[commit-bot: I haz the power, 2013-03-26 21:38:17]

Change committed as 190772