Purge legacy notification promo code.

NotificationPromo refactoring.

* Purge legacy notification promo code.
* Get rid of NotificationPromo::Delegate.
* Get rid of RefCountedThreadSafe.
* Unit test cleanup.


BUG=12637
TEST=compiles, unit tests pass.

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

[achuithb, 2012-06-02 00:17:31]

Dan, please review.

[Dan Beam, 2012-06-04 18:20:08]

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/notification_promo.cc (right):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/notification_promo.cc:330: double
NotificationPromo::EndTime() const {
you only use this once, btw...

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:57: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kPromoServerURL);
is it time to add something else here that delays possibly pinging the promo
server during every automated test?

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:28: // TODO(mirandac):
Arrange for a server to be set up specifically for promo
this is TODONE()

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (left):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:353: void
TestOnePlatform(int current_platform, bool expected) {
so you've removed all these because this is done on the server now? don't we
need to continue supporting the legacy version for now, or conversely remove all
the code?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:494: //
TODO(achuith): Test this better.
can you add something here in the future (or now, whatevs) that shoves the URL
into GURL() and checks that the protocol is "https"?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:497:
TEST_F(PromoResourceServiceTest, UnpackWebStoreSignal) {
same overall question about legacy vs. new: do we still use the web store
signal/logo stuff?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:721:
TEST_F(PromoResourceServiceTest, IsBuildTargetedTest) {
do we still do this on the client?

[Dan Beam, 2012-06-04 18:41:04]

do these in a separate CL

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:34: static const int
kTestCacheUpdateDelay = 3 * 60 * 1000;
I think this should be a separate flag, personally, so we can change it to less
without modifying the source (and essentially turn it off for tests by making it
really large or something...) or at least add some type of --enable-promos
(which would make tin foils hat wearers and unit tests happy!).

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:61: return IsTest() ?
kTestCacheUpdateDelay : kCacheUpdateDelay;
I'd prefer to separate this logic / control this with more granularity

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:67: const char*
PromoResourceService::kDefaultPromoResourceServer =
legacy

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:74:
local_state->RegisterIntegerPref(prefs::kNtpPromoVersion, 0);
is this still truly global, or can there be different versions per profile?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:95:
base::ThreadRestrictions::ScopedAllowIO allow_io;
would it be worth doing an #if defined(OS_WINDOWS) here then (or preferably
lower in the code in GetChannel() perhaps?)

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:100: bool
PromoResourceService::IsBuildTargeted(chrome::VersionInfo::Channel channel,
why do we still need this? or perhaps only keep the flag check?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:175: if
((GetPromoServiceVersion() != kPromoServiceVersion) ||
less ()

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:204: } else if (delay_ms
== 0) {
I think we should still post a task with 0 ms isntead of doing it in a different
way here

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:245: void
PromoResourceService::UnpackWebStoreSignal(
if we're not using the tips server, how is any of this used / functional any
more?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:319: if (!signal_found) {
move comment out and remove curlies, IMO

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:329: void
PromoResourceService::UnpackLogoSignal(
same story here with legacy code probably not being used/working any more

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:358: if (logo_signal ==
"custom_logo_start") {
no curlies

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:384: if (!(old_logo_start
== logo_start) ||
demorgan's law + less ()

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:117: //         "question":
"1:24:10:20:7:0",
none of this is the case any more, right?

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:145: // group that should
see it, the maximum number of views of the promo,the
and for the love of all things holy, if we're keeping this text add a space
after the comma here:

  promo,the

it's always annoyed the hell out of me.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:43:
TEST_F(PromoResourceServiceTest, UnpackLogoSignal) {
don't seem relevant any more

[achuithb, 2012-06-04 19:52:59]

Hope I didn't miss any comments. PTAL.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/notification_promo.cc (right):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/notification_promo.cc:330: double
NotificationPromo::EndTime() const {
On 2012/06/04 18:20:08, Dan Beam wrote:
> you only use this once, btw...

It's used in PromoResourceService::UnpackNotificationSignal()

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:34: static const int
kTestCacheUpdateDelay = 3 * 60 * 1000;
On 2012/06/04 18:41:04, Dan Beam wrote:
> I think this should be a separate flag, personally, so we can change it to
less
> without modifying the source (and essentially turn it off for tests by making
it
> really large or something...) or at least add some type of --enable-promos
> (which would make tin foils hat wearers and unit tests happy!).

We probably already have too many chrome flags. We should just pick a good debug
value. If 3 min is too long, maybe we should reduce this?

As for unit tests, I think we had a discussion about this (can't remember if it
was me and Rob or me and you?), and concluded that we don't want to do anything
special for tests. In other similar situations, I think we continue to hit
production and the load from tests ends up being is relatively small. We also
decided that we didn't want more code that behaves differently in a test
situation.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:57: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kPromoServerURL);
On 2012/06/04 18:20:08, Dan Beam wrote:
> is it time to add something else here that delays possibly pinging the promo
> server during every automated test?

Let's punt this for now.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:61: return IsTest() ?
kTestCacheUpdateDelay : kCacheUpdateDelay;
On 2012/06/04 18:41:04, Dan Beam wrote:
> I'd prefer to separate this logic / control this with more granularity

Punt for now.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:67: const char*
PromoResourceService::kDefaultPromoResourceServer =
On 2012/06/04 18:41:04, Dan Beam wrote:
> legacy

Done.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:74:
local_state->RegisterIntegerPref(prefs::kNtpPromoVersion, 0);
On 2012/06/04 18:41:04, Dan Beam wrote:
> is this still truly global, or can there be different versions per profile?

It makes sense for it to be truly global, but I don't think this code quite
works since we only clear the current profile's prefs. 

I'll add a TODO.

FWIW, I don't think we've changed the promo version for a while.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:95:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/06/04 18:41:04, Dan Beam wrote:
> would it be worth doing an #if defined(OS_WINDOWS) here then (or preferably
> lower in the code in GetChannel() perhaps?)

Hopefully we can delete this.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:100: bool
PromoResourceService::IsBuildTargeted(chrome::VersionInfo::Channel channel,
On 2012/06/04 18:41:04, Dan Beam wrote:
> why do we still need this? or perhaps only keep the flag check?

flag check?

It's used in UnpackWebstoreSignal

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:175: if
((GetPromoServiceVersion() != kPromoServiceVersion) ||
On 2012/06/04 18:41:04, Dan Beam wrote:
> less ()

Done.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:204: } else if (delay_ms
== 0) {
On 2012/06/04 18:41:04, Dan Beam wrote:
> I think we should still post a task with 0 ms isntead of doing it in a
different
> way here

I tried that, and it crashes on Init, possibly because something is not set up
yet? I'd rather not investigate this now, so leaving it as is.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:245: void
PromoResourceService::UnpackWebStoreSignal(
On 2012/06/04 18:41:04, Dan Beam wrote:
> if we're not using the tips server, how is any of this used / functional any
> more?

Chatted about this; Tips server continues to be used for WebStoreSignal and
LogoSignal. Not sure what the status of those are. Will send an email about this
and deal with this in a separate CL.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:319: if (!signal_found) {
On 2012/06/04 18:41:04, Dan Beam wrote:
> move comment out and remove curlies, IMO

Hopefully we'll delete this code.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:329: void
PromoResourceService::UnpackLogoSignal(
On 2012/06/04 18:41:04, Dan Beam wrote:
> same story here with legacy code probably not being used/working any more

Will send an email, etc.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:358: if (logo_signal ==
"custom_logo_start") {
On 2012/06/04 18:41:04, Dan Beam wrote:
> no curlies

Hopefully delete this

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:384: if (!(old_logo_start
== logo_start) ||
On 2012/06/04 18:41:04, Dan Beam wrote:
> demorgan's law + less ()

hopefully delete this.

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:28: // TODO(mirandac):
Arrange for a server to be set up specifically for promo
On 2012/06/04 18:20:08, Dan Beam wrote:
> this is TODONE()

Done.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:117: //         "question":
"1:24:10:20:7:0",
On 2012/06/04 18:41:04, Dan Beam wrote:
> none of this is the case any more, right?

Done.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:145: // group that should
see it, the maximum number of views of the promo,the
On 2012/06/04 18:41:04, Dan Beam wrote:
> and for the love of all things holy, if we're keeping this text add a space
> after the comma here:
> 
>   promo,the
> 
> it's always annoyed the hell out of me.

Comments are no longer valid (thanks). I've deleted them. This is probably not
the best place to describe the json format.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (left):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:353: void
TestOnePlatform(int current_platform, bool expected) {
On 2012/06/04 18:20:08, Dan Beam wrote:
> so you've removed all these because this is done on the server now? don't we
> need to continue supporting the legacy version for now, or conversely remove
all
> the code?

The build/platform checks are now done for notification promos on the server
(which is what this tests). logo/webstore signals continue to do the client side
checks.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:43:
TEST_F(PromoResourceServiceTest, UnpackLogoSignal) {
On 2012/06/04 18:41:04, Dan Beam wrote:
> don't seem relevant any more

Code is not yet deprecated.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:494: //
TODO(achuith): Test this better.
On 2012/06/04 18:20:08, Dan Beam wrote:
> can you add something here in the future (or now, whatevs) that shoves the URL
> into GURL() and checks that the protocol is "https"?

Done.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:497:
TEST_F(PromoResourceServiceTest, UnpackWebStoreSignal) {
On 2012/06/04 18:20:08, Dan Beam wrote:
> same overall question about legacy vs. new: do we still use the web store
> signal/logo stuff?

Answered elsewhere

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:721:
TEST_F(PromoResourceServiceTest, IsBuildTargetedTest) {
On 2012/06/04 18:20:08, Dan Beam wrote:
> do we still do this on the client?

Yup, for webstore/logo

[Dan Beam, 2012-06-04 21:26:32]

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:100: bool
PromoResourceService::IsBuildTargeted(chrome::VersionInfo::Channel channel,
On 2012/06/04 19:52:59, achuith.bhandarkar wrote:
> On 2012/06/04 18:41:04, Dan Beam wrote:
> > why do we still need this? or perhaps only keep the flag check?
>
> flag check?

  return CommandLine::ForCurrentProcess()->HasSwitch(
      switches::kPromoServerURL);

> It's used in UnpackWebstoreSignal

I meant do we use ^ that? It seems that you think we still need it.

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:204: } else if (delay_ms
== 0) {
On 2012/06/04 19:52:59, achuith.bhandarkar wrote:
> On 2012/06/04 18:41:04, Dan Beam wrote:
> > I think we should still post a task with 0 ms isntead of doing it in a
> different
> > way here
> 
> I tried that, and it crashes on Init, possibly because something is not set up
> yet? I'd rather not investigate this now, so leaving it as is.

TODO() or comment, then?

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
File chrome/browser/web_resource/promo_resource_service.cc (left):

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
chrome/browser/web_resource/promo_resource_service.cc:66: // Server for
dynamically loaded NTP HTML elements.
at the moment this is the URL we can get the webstore logo from, and as you've
said many times we still need that code, so why are you removing this? something
I'm missing?

[achuithb, 2012-06-04 23:06:08]

Will have round #2 of purging after this lands, to get rid of Webstore/Logo
signals.

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

http://codereview.chromium.org/10496008/diff/6003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:204: } else if (delay_ms
== 0) {
On 2012/06/04 21:26:32, Dan Beam wrote:
> On 2012/06/04 19:52:59, achuith.bhandarkar wrote:
> > On 2012/06/04 18:41:04, Dan Beam wrote:
> > > I think we should still post a task with 0 ms isntead of doing it in a
> > different
> > > way here
> > 
> > I tried that, and it crashes on Init, possibly because something is not set
up
> > yet? I'd rather not investigate this now, so leaving it as is.
> 
> TODO() or comment, then?

Done.

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
File chrome/browser/web_resource/promo_resource_service.cc (left):

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
chrome/browser/web_resource/promo_resource_service.cc:66: // Server for
dynamically loaded NTP HTML elements.
On 2012/06/04 21:26:32, Dan Beam wrote:
> at the moment this is the URL we can get the webstore logo from, and as you've
> said many times we still need that code, so why are you removing this?
something
> I'm missing?

You're right. So there is no access to the tips server, and hasn't been in M20.
There isn't a way to serve webstore/logo promos any more.

[Dan Beam, 2012-06-05 02:53:13]

lgtm, but remove the rest before we forget about it, :P

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
File chrome/browser/web_resource/promo_resource_service.h (left):

http://codereview.chromium.org/10496008/diff/15002/chrome/browser/web_resourc...
chrome/browser/web_resource/promo_resource_service.h:28: // TODO(mirandac):
Arrange for a server to be set up specifically for promo
huzzah!

[commit-bot: I haz the power, 2012-06-05 07:46:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10496008/13004

[commit-bot: I haz the power, 2012-06-05 09:56:08]

Change committed as 140511