Update the web store promo to be clearer and configurable at run-time.

Update the web store promo to be clearer and configurable at run-time.

The promo now behaves like this:
a) Promo is fetched 5 seconds after first launch.
b) Promo is shown whenever no apps are installed, or if only default apps are installed (from old Chrome versions).
c) Clicking "hide this" puts the apps section into menu mode.
d) Switching locales will fetch a new promo in that locale.
e) We no longer install default apps, but they'll expire and be uninstalled the same as before.

BUG=78358
TEST=PromoResourceServiceTest, ExtensionAppsPromo

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

[jstritar, 2011-04-12 21:43:07]

The new promo is ready to review. I was thinking something like this for
reviewers:

arv: web / promo resource and ntp work
aa: extensions and ntp work

I added Miranda as well since she has a pending CL for PromoResourceService. I
hope there won't be too many things to merge.

Thanks!

[Aaron Boodman, 2011-04-12 22:54:03]

I have no time to review this right now. Arv, can you take my bit? It does not
seem extension-specific.

[Miranda Callahan, 2011-04-13 14:29:29]

LGTM with nits and nonsubstantial comments from me. Thank you for the PRS
cleanup!

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:19: return
g_browser_process->local_state();
I would remove this whole anonymous namespace and one-line function, and just
call g_browser_process->local_state() wherever you need to. This is common
throughout the chromium code base, and doesn't need to be encapsulated in a
function.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:114: // Poppit, Entanglement
Thanks for the comment -- I am not familiar with how you determine default apps,
but should these apps be taken from some central place that you can call to get
a vector of "DefaultAppIdStrings", or something?  My worry is that the default
apps will change somewhere else, but not be changed here, and things will get
out of sync.  Perhaps this worry is overblown.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:126: switches::kForceAppsPromoVisible))
{
indent more nicely

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.h (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:12: #include "base/gtest_prod_util.h"
nit: alphabetical order, and empty line between c++ library and chrome
libraries.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:26: // Remoes the current promo data.
s/Remoes/Removes.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.js:1470: // Initialize the apps promo.
should this comment change now as well?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:122: channel =
platform_util::GetVersionStringModifier();
did you want to cache the channel in channel_ here?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:303: for
(ListValue::const_iterator tip_iter = answer_list->begin();
can you change the name from the atavistic "tip_iter" to something more
reflective of what is being iterated through? :-)

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.h:48: // Identifies types of
Chrome builds for promo targetting.
s/targetting/targeting

[arv (Not doing code reviews), 2011-04-13 17:17:59]

I only looked at the html part for now.

Jon, do you want me to look at the C++ parts too?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.html:151: <div><span><span>
div-span-span? What tag is that?

Why do you need these and why isn't a plain <a> enough?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp/apps.css (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:147: padding-left: 125px;
-webkit-padding-start

Any time you see left and right in css you need to do RTL

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:156: margin-left: 3px;
-webkit-margin-start

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:191: /* g-button CSS styles */
I have no idea what this is? Is this some shared css? If so move to a shared
file?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:212: background:
url('g-button-chocobo-basic-1.png') no-repeat;
-webkit-border-image FTW. You can do this fancy button with one <a> and one
small image in modern browsers.

http://www.xanthir.com/blog/b49y0

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:219: /* @noflip */ background:
url('g-button-chocobo-basic-2.png') no-repeat right bottom;
remove GSS annotations?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:223: height:  1%;
ws

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:244: /* @noflip */ background:
url('g-button-chocobo-basic-1.png') no-repeat left bottom;
This will duplicate the data due to inlining. Can you refactor?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:69: appsPromoLink.setAttribute('ping',
appsPromoPing);
I added the ping dom binding 2 weeks ago. Can you update the js to use that
while you are here?

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:193: return (getAppLaunchType() ==
APP_LAUNCH.NTP_APPS_MAXIMIZED) &&
useless parentheses

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:371: if (promoHasBeenSeen) return;
if (...)
  return;

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:372: if (!this.showPromo ||
!isAppSectionMaximized()) return;
merge if statements?

[jstritar, 2011-04-13 19:52:27]

I think Miranda looked at the C++. Miranda, is the C++ good to go or should Erik
take a look too?

I just uploaded a patch that addresses the feedback (with a couple caveats
below). It also updates the feed format, based on limitations I ran into on the
real server (it's working against live promo data now). I also removed the most
visited sections tip that appears on first run of the NTP (it no longer makes
sense since the apps section will be maximized). I didn't remove the strings for
it in case there's push to add it back.

Erik, I didn't make changes to the button since I imported from go/buttons. I'm
also not very familiar with how these background images work (using them for
borders), so it will take a little while for me to refractor it. If you still
think I should, I'm fine with it though! (I also heard they're redesigning them,
so we'll probably be replacing it in a couple months).

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:19: return
g_browser_process->local_state();
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> I would remove this whole anonymous namespace and one-line function, and just
> call g_browser_process->local_state() wherever you need to. This is common
> throughout the chromium code base, and doesn't need to be encapsulated in a
> function.

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:114: // Poppit, Entanglement
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> Thanks for the comment -- I am not familiar with how you determine default
apps,
> but should these apps be taken from some central place that you can call to
get
> a vector of "DefaultAppIdStrings", or something?  My worry is that the default
> apps will change somewhere else, but not be changed here, and things will get
> out of sync.  Perhaps this worry is overblown.

This is actually the one place they're defined. The class used to be called
DefaultApps, and it managed the default apps installation and apps promo. With
the promo redesign, we no longer have default apps, so the class is mainly
focused on the promo (which is why I renamed it). We still need to keep track of
the default apps that may have been installed on previous Chrome versions
because we automatically uninstall them under certain scenarios.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:126: switches::kForceAppsPromoVisible))
{
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> indent more nicely

I think this is the correct indentation because the conditional doesn't fit on
one line and the argument is indented 4 spaces from the start of "CommandLine".
What do you have in mind? I'm open to suggestions, cause it doesn't look that
nice.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.h (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:12: #include "base/gtest_prod_util.h"
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> nit: alphabetical order, and empty line between c++ library and chrome
> libraries.

Woops, thanks. Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:26: // Remoes the current promo data.
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> s/Remoes/Removes.  

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.html:151: <div><span><span>
On 2011/04/13 17:17:59, arv wrote:
> div-span-span? What tag is that?
> 
> Why do you need these and why isn't a plain <a> enough?

I asked myself the same question. I'm using the buttons from go/buttons. I'm not
sure its worth modifying them much, since I'd like to make it easy to port
changes in them back to Chrome.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.js:1470: // Initialize the apps promo.
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> should this comment change now as well?

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp/apps.css (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:147: padding-left: 125px;
On 2011/04/13 17:17:59, arv wrote:
> -webkit-padding-start
> 
> Any time you see left and right in css you need to do RTL

Done. Thanks for catching that. Also, didn't know about the -webkit-*-start
rules... very cool.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:156: margin-left: 3px;
On 2011/04/13 17:17:59, arv wrote:
> -webkit-margin-start

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.css:219: /* @noflip */ background:
url('g-button-chocobo-basic-2.png') no-repeat right bottom;
On 2011/04/13 17:17:59, arv wrote:
> remove GSS annotations?

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:69: appsPromoLink.setAttribute('ping',
appsPromoPing);
On 2011/04/13 17:17:59, arv wrote:
> I added the ping dom binding 2 weeks ago. Can you update the js to use that
> while you are here?

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:193: return (getAppLaunchType() ==
APP_LAUNCH.NTP_APPS_MAXIMIZED) &&
On 2011/04/13 17:17:59, arv wrote:
> useless parentheses

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:371: if (promoHasBeenSeen) return;
On 2011/04/13 17:17:59, arv wrote:
> if (...)
>   return;

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:372: if (!this.showPromo ||
!isAppSectionMaximized()) return;
On 2011/04/13 17:17:59, arv wrote:
> merge if statements?

Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:122: channel =
platform_util::GetVersionStringModifier();
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> did you want to cache the channel in channel_ here?

Oh, yeah! Done.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:303: for
(ListValue::const_iterator tip_iter = answer_list->begin();
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> can you change the name from the atavistic "tip_iter" to something more
> reflective of what is being iterated through? :-)

Done. I changed them to "answer_iter". Another option could be "signal_iter"
since the methods are named UnpackWebStoreSignal, etc.

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.h:48: // Identifies types of
Chrome builds for promo targetting.
On 2011/04/13 14:29:29, Miranda Callahan wrote:
> s/targetting/targeting

Done.

[mirandac, 2011-04-13 23:35:08]

C++ related to the resource service side is LGTM from me -- arv can
review as he prefers.

Sent from my iPhone

On Apr 13, 2011, at 2:52 PM, "jstritar@chromium.org"
<jstritar@chromium.org> wrote:

> I think Miranda looked at the C++. Miranda, is the C++ good to go or should
Erik
> take a look too?
>
> I just uploaded a patch that addresses the feedback (with a couple caveats
> below). It also updates the feed format, based on limitations I ran into on
the
> real server (it's working against live promo data now). I also removed the
most
> visited sections tip that appears on first run of the NTP (it no longer makes
> sense since the apps section will be maximized). I didn't remove the strings
for
> it in case there's push to add it back.
>
> Erik, I didn't make changes to the button since I imported from go/buttons.
I'm
> also not very familiar with how these background images work (using them for
> borders), so it will take a little while for me to refractor it. If you still
> think I should, I'm fine with it though! (I also heard they're redesigning
them,
> so we'll probably be replacing it in a couple months).
>
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> File chrome/browser/extensions/apps_promo.cc (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> chrome/browser/extensions/apps_promo.cc:19: return
> g_browser_process->local_state();
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> I would remove this whole anonymous namespace and one-line function,
> and just
>> call g_browser_process->local_state() wherever you need to. This is
> common
>> throughout the chromium code base, and doesn't need to be encapsulated
> in a
>> function.
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> chrome/browser/extensions/apps_promo.cc:114: // Poppit, Entanglement
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> Thanks for the comment -- I am not familiar with how you determine
> default apps,
>> but should these apps be taken from some central place that you can
> call to get
>> a vector of "DefaultAppIdStrings", or something?  My worry is that the
> default
>> apps will change somewhere else, but not be changed here, and things
> will get
>> out of sync.  Perhaps this worry is overblown.
>
> This is actually the one place they're defined. The class used to be
> called DefaultApps, and it managed the default apps installation and
> apps promo. With the promo redesign, we no longer have default apps, so
> the class is mainly focused on the promo (which is why I renamed it). We
> still need to keep track of the default apps that may have been
> installed on previous Chrome versions because we automatically uninstall
> them under certain scenarios.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> chrome/browser/extensions/apps_promo.cc:126:
> switches::kForceAppsPromoVisible)) {
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> indent more nicely
>
> I think this is the correct indentation because the conditional doesn't
> fit on one line and the argument is indented 4 spaces from the start of
> "CommandLine". What do you have in mind? I'm open to suggestions, cause
> it doesn't look that nice.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> File chrome/browser/extensions/apps_promo.h (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> chrome/browser/extensions/apps_promo.h:12: #include
> "base/gtest_prod_util.h"
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> nit: alphabetical order, and empty line between c++ library and chrome
>> libraries.
>
> Woops, thanks. Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/extensions/ap...
> chrome/browser/extensions/apps_promo.h:26: // Remoes the current promo
> data.
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> s/Remoes/Removes.
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
> File chrome/browser/resources/new_new_tab.html (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
> chrome/browser/resources/new_new_tab.html:151: <div><span><span>
> On 2011/04/13 17:17:59, arv wrote:
>> div-span-span? What tag is that?
>
>> Why do you need these and why isn't a plain <a> enough?
>
> I asked myself the same question. I'm using the buttons from go/buttons.
> I'm not sure its worth modifying them much, since I'd like to make it
> easy to port changes in them back to Chrome.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
> File chrome/browser/resources/new_new_tab.js (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/new...
> chrome/browser/resources/new_new_tab.js:1470: // Initialize the apps
> promo.
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> should this comment change now as well?
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> File chrome/browser/resources/ntp/apps.css (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.css:147: padding-left: 125px;
> On 2011/04/13 17:17:59, arv wrote:
>> -webkit-padding-start
>
>> Any time you see left and right in css you need to do RTL
>
> Done. Thanks for catching that. Also, didn't know about the
> -webkit-*-start rules... very cool.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.css:156: margin-left: 3px;
> On 2011/04/13 17:17:59, arv wrote:
>> -webkit-margin-start
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.css:219: /* @noflip */ background:
> url('g-button-chocobo-basic-2.png') no-repeat right bottom;
> On 2011/04/13 17:17:59, arv wrote:
>> remove GSS annotations?
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> File chrome/browser/resources/ntp/apps.js (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.js:69:
> appsPromoLink.setAttribute('ping', appsPromoPing);
> On 2011/04/13 17:17:59, arv wrote:
>> I added the ping dom binding 2 weeks ago. Can you update the js to use
> that
>> while you are here?
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.js:193: return (getAppLaunchType() ==
> APP_LAUNCH.NTP_APPS_MAXIMIZED) &&
> On 2011/04/13 17:17:59, arv wrote:
>> useless parentheses
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.js:371: if (promoHasBeenSeen) return;
> On 2011/04/13 17:17:59, arv wrote:
>> if (...)
>>   return;
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp/apps.js:372: if (!this.showPromo ||
> !isAppSectionMaximized()) return;
> On 2011/04/13 17:17:59, arv wrote:
>> merge if statements?
>
> Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
> File chrome/browser/web_resource/promo_resource_service.cc (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
> chrome/browser/web_resource/promo_resource_service.cc:122: channel =
> platform_util::GetVersionStringModifier();
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> did you want to cache the channel in channel_ here?
>
> Oh, yeah! Done.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
> chrome/browser/web_resource/promo_resource_service.cc:303: for
> (ListValue::const_iterator tip_iter = answer_list->begin();
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> can you change the name from the atavistic "tip_iter" to something
> more
>> reflective of what is being iterated through? :-)
>
> Done. I changed them to "answer_iter". Another option could be
> "signal_iter" since the methods are named UnpackWebStoreSignal, etc.
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
> File chrome/browser/web_resource/promo_resource_service.h (right):
>
>
http://codereview.chromium.org/6825052/diff/7001/chrome/browser/web_resource/...
> chrome/browser/web_resource/promo_resource_service.h:48: // Identifies
> types of Chrome builds for promo targetting.
> On 2011/04/13 14:29:29, Miranda Callahan wrote:
>> s/targetting/targeting
>
> Done.
>
> http://codereview.chromium.org/6825052/

[arv (Not doing code reviews), 2011-04-14 05:26:11]

HTML/JS/CSS LGTM with the bug fixed

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/resources/ne...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:90: a.setAttribute.ping = opt_pingUrl;
Ooops!

a.ping = opt_pingUrl;

[Aaron Boodman, 2011-04-14 19:07:09]

lgtm w/ some naming suggestions

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/extensions/a...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/extensions/a...
chrome/browser/extensions/apps_promo.cc:20: std::string str;
Perhaps call this 'empty' or 'default' to make more clear what you're doing?

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/extensions/a...
chrome/browser/extensions/apps_promo.cc:41:
local_state->ClearPref(prefs::kNTPWebStorePromoId);
If you grouped these preferences under a 'webstorepromo.' parent, it would be
easier and less error-prone to clear them all together.

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/extensions/a...
File chrome/browser/extensions/apps_promo.h (right):

http://codereview.chromium.org/6825052/diff/16001/chrome/browser/extensions/a...
chrome/browser/extensions/apps_promo.h:106: ExtensionIdSet ids_;
Since the primary purpose of this class is no longer to install default apps,
consider renaming this old_default_app_ids, or something.

[jstritar, 2011-04-14 19:58:59]

thank you for the feedback everyone!

[jstritar, 2011-04-14 22:12:12]

Hi Brett, could you take a look at the change to
content/common/notification_type.h (you're in the OWNERS file)?

Thanks!

[brettw, 2011-04-15 15:51:15]

content LGTM