Purge Logo/WebstoreSignal.

Purge Logo/WebstoreSignal.

BUG=NONE
TEST=unit tests pass.
R=dbeam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141183

[achuithb, 2012-06-07 11:16:29]

Please review. Swathes of deleted code - yay!

[achuithb, 2012-06-07 22:30:26]

On 2012/06/07 11:16:29, achuith.bhandarkar wrote:
> Please review. Swathes of deleted code - yay!

ping?

[Dan Beam, 2012-06-07 22:35:01]

Busy

[aruslan, 2012-06-07 23:14:00]

LGTM
(I'm not a committer.)

[commit-bot: I haz the power, 2012-06-08 07:48:21]

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

[commit-bot: I haz the power, 2012-06-08 07:48:26]

Presubmit check for 10539045-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    chrome/browser/web_resource/notification_promo.cc:54

[Dan Beam, 2012-06-08 18:49:47]

who's gonna delete the stuff in extensions and NTP?

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:54:
base::ThreadRestrictions::ScopedAllowIO allow_io;
presubmit is angry at you for this ^, also

#if defined(OS_WIN)
  base::ThreadRestrictions::ScopedAllowIO allow_io;
#endif

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/promo_resource_service.cc:64: // TODO(achuith):
Delete this in M21
M21 or M22? . and end.

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/promo_resource_service.h (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/promo_resource_service.h:26: class
PromoResourceService : public WebResourceService {
yay, it looks like a normal header now!

[achuithb, 2012-06-08 22:21:28]

On 2012/06/08 18:49:47, Dan Beam wrote:
> who's gonna delete the stuff in extensions and NTP?
> 

I'll take care of it in a bit.

[achuithb, 2012-06-08 22:29:08]

I'll address these in a new CL. Thank for the comments, Dan.

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:54:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/06/08 18:49:47, Dan Beam wrote:
> presubmit is angry at you for this ^, also
> 
> #if defined(OS_WIN)
>   base::ThreadRestrictions::ScopedAllowIO allow_io;
> #endif

I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by this patch,
but was moved from PromoResourceService.

I thought about this and it's going to be a decent amount of work to eliminate
this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is needed in
the ctor of WebResourceService. We'd need to refactor all that stuff to take
callbacks, and I'd rather not do that now.

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/promo_resource_service.cc:64: // TODO(achuith):
Delete this in M21
On 2012/06/08 18:49:47, Dan Beam wrote:
> M21 or M22? . and end.

Will fix.

[Dan Beam, 2012-06-08 22:34:54]

you committed before I LG'd and I was reviewing (not TBR'ing)

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:54:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> On 2012/06/08 18:49:47, Dan Beam wrote:
> > presubmit is angry at you for this ^, also
> > 
> > #if defined(OS_WIN)
> >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > #endif
> 
> I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by this
patch,
> but was moved from PromoResourceService.
> 
> I thought about this and it's going to be a decent amount of work to eliminate
> this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is needed
in
> the ctor of WebResourceService. We'd need to refactor all that stuff to take
> callbacks, and I'd rather not do that now.

I still think you should move this #ifdef inside
chrome::VersionInfo::GetChannel().

[Robert Sesek, 2012-06-08 22:35:53]

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:54:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/06/08 22:34:54, Dan Beam wrote:
> On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > presubmit is angry at you for this ^, also
> > > 
> > > #if defined(OS_WIN)
> > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > #endif
> > 
> > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by this
> patch,
> > but was moved from PromoResourceService.
> > 
> > I thought about this and it's going to be a decent amount of work to
eliminate
> > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is
needed
> in
> > the ctor of WebResourceService. We'd need to refactor all that stuff to take
> > callbacks, and I'd rather not do that now.
> 
> I still think you should move this #ifdef inside
> chrome::VersionInfo::GetChannel().

I disagree with this. Clients of this function should know explicitly that it
does IO.

[Dan Beam, 2012-06-08 22:36:56]

On 2012/06/08 22:35:53, rsesek wrote:
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> File chrome/browser/web_resource/notification_promo.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> chrome/browser/web_resource/notification_promo.cc:54:
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> On 2012/06/08 22:34:54, Dan Beam wrote:
> > On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > > presubmit is angry at you for this ^, also
> > > > 
> > > > #if defined(OS_WIN)
> > > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > #endif
> > > 
> > > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by this
> > patch,
> > > but was moved from PromoResourceService.
> > > 
> > > I thought about this and it's going to be a decent amount of work to
> eliminate
> > > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is
> needed
> > in
> > > the ctor of WebResourceService. We'd need to refactor all that stuff to
take
> > > callbacks, and I'd rather not do that now.
> > 
> > I still think you should move this #ifdef inside
> > chrome::VersionInfo::GetChannel().
> 
> I disagree with this. Clients of this function should know explicitly that it
> does IO.

It would be better to just make it an async API and/or cache the value on
startup, IMO.

[Robert Sesek, 2012-06-08 22:37:54]

On 2012/06/08 22:36:56, Dan Beam wrote:
> On 2012/06/08 22:35:53, rsesek wrote:
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > File chrome/browser/web_resource/notification_promo.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > chrome/browser/web_resource/notification_promo.cc:54:
> > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > On 2012/06/08 22:34:54, Dan Beam wrote:
> > > On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > > > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > > > presubmit is angry at you for this ^, also
> > > > > 
> > > > > #if defined(OS_WIN)
> > > > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > > #endif
> > > > 
> > > > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by
this
> > > patch,
> > > > but was moved from PromoResourceService.
> > > > 
> > > > I thought about this and it's going to be a decent amount of work to
> > eliminate
> > > > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is
> > needed
> > > in
> > > > the ctor of WebResourceService. We'd need to refactor all that stuff to
> take
> > > > callbacks, and I'd rather not do that now.
> > > 
> > > I still think you should move this #ifdef inside
> > > chrome::VersionInfo::GetChannel().
> > 
> > I disagree with this. Clients of this function should know explicitly that
it
> > does IO.
> 
> It would be better to just make it an async API and/or cache the value on
> startup, IMO.

Definitely agree, but Achuith said (and I agree) that that's work outside of
this CL. I just don't want clients implicitly doing IO on a thread on which it
shouldn't be done.

[Dan Beam, 2012-06-08 22:39:18]

On 2012/06/08 22:37:54, rsesek wrote:
> On 2012/06/08 22:36:56, Dan Beam wrote:
> > On 2012/06/08 22:35:53, rsesek wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > File chrome/browser/web_resource/notification_promo.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > chrome/browser/web_resource/notification_promo.cc:54:
> > > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > On 2012/06/08 22:34:54, Dan Beam wrote:
> > > > On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > > > > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > > > > presubmit is angry at you for this ^, also
> > > > > > 
> > > > > > #if defined(OS_WIN)
> > > > > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > > > #endif
> > > > > 
> > > > > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by
> this
> > > > patch,
> > > > > but was moved from PromoResourceService.
> > > > > 
> > > > > I thought about this and it's going to be a decent amount of work to
> > > eliminate
> > > > > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is
> > > needed
> > > > in
> > > > > the ctor of WebResourceService. We'd need to refactor all that stuff
to
> > take
> > > > > callbacks, and I'd rather not do that now.
> > > > 
> > > > I still think you should move this #ifdef inside
> > > > chrome::VersionInfo::GetChannel().
> > > 
> > > I disagree with this. Clients of this function should know explicitly that
> it
> > > does IO.
> > 
> > It would be better to just make it an async API and/or cache the value on
> > startup, IMO.
> 
> Definitely agree, but Achuith said (and I agree) that that's work outside of
> this CL. I just don't want clients implicitly doing IO on a thread on which it
> shouldn't be done.

I don't think it's very good API to make all callers know about the side effects
of calling it.

[Dan Beam, 2012-06-08 22:40:05]

On 2012/06/08 22:39:18, Dan Beam wrote:
> On 2012/06/08 22:37:54, rsesek wrote:
> > On 2012/06/08 22:36:56, Dan Beam wrote:
> > > On 2012/06/08 22:35:53, rsesek wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > > File chrome/browser/web_resource/notification_promo.cc (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > > chrome/browser/web_resource/notification_promo.cc:54:
> > > > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > On 2012/06/08 22:34:54, Dan Beam wrote:
> > > > > On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > > > > > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > > > > > presubmit is angry at you for this ^, also
> > > > > > > 
> > > > > > > #if defined(OS_WIN)
> > > > > > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > > > > #endif
> > > > > > 
> > > > > > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by
> > this
> > > > > patch,
> > > > > > but was moved from PromoResourceService.
> > > > > > 
> > > > > > I thought about this and it's going to be a decent amount of work to
> > > > eliminate
> > > > > > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which
is
> > > > needed
> > > > > in
> > > > > > the ctor of WebResourceService. We'd need to refactor all that stuff
> to
> > > take
> > > > > > callbacks, and I'd rather not do that now.
> > > > > 
> > > > > I still think you should move this #ifdef inside
> > > > > chrome::VersionInfo::GetChannel().
> > > > 
> > > > I disagree with this. Clients of this function should know explicitly
that
> > it
> > > > does IO.
> > > 
> > > It would be better to just make it an async API and/or cache the value on
> > > startup, IMO.
> > 
> > Definitely agree, but Achuith said (and I agree) that that's work outside of
> > this CL. I just don't want clients implicitly doing IO on a thread on which
it
> > shouldn't be done.
> 
> I don't think it's very good API to make all callers know about the side
effects
> of calling it.

(in a sense that they must make a scoped io allowance in their *code*, not that
the shouldn't be aware it might take a while or is slow)

[achuithb, 2012-06-08 22:48:57]

On 2012/06/08 22:34:54, Dan Beam wrote:
> you committed before I LG'd and I was reviewing (not TBR'ing)
> 

I got an LGTM from Aruslan, who's working on Clank (he's not a chromium
committer yet), and he did take a look at it.

I did ping you and you said you were busy. There is a hard M21 deadline and
unfortunately I can't afford to wait multiple days waiting on reviews for
cleanup CLs that are blocking future work. Fwiw, my manager asked me to wait for
24 hours for an lgtm and then escalate or TBR. In this case, the code was
reviewed and I only needed an owner lgtm from you.

[Robert Sesek, 2012-06-08 22:50:48]

On 2012/06/08 22:40:05, Dan Beam wrote:
> On 2012/06/08 22:39:18, Dan Beam wrote:
> > On 2012/06/08 22:37:54, rsesek wrote:
> > > On 2012/06/08 22:36:56, Dan Beam wrote:
> > > > On 2012/06/08 22:35:53, rsesek wrote:
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > > > File chrome/browser/web_resource/notification_promo.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> > > > > chrome/browser/web_resource/notification_promo.cc:54:
> > > > > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > > On 2012/06/08 22:34:54, Dan Beam wrote:
> > > > > > On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > > > > > > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > > > > > > presubmit is angry at you for this ^, also
> > > > > > > > 
> > > > > > > > #if defined(OS_WIN)
> > > > > > > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > > > > > > #endif
> > > > > > > 
> > > > > > > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced
by
> > > this
> > > > > > patch,
> > > > > > > but was moved from PromoResourceService.
> > > > > > > 
> > > > > > > I thought about this and it's going to be a decent amount of work
to
> > > > > eliminate
> > > > > > > this ScopedAllowIO. channel is needed by GetPromoServerUrl(),
which
> is
> > > > > needed
> > > > > > in
> > > > > > > the ctor of WebResourceService. We'd need to refactor all that
stuff
> > to
> > > > take
> > > > > > > callbacks, and I'd rather not do that now.
> > > > > > 
> > > > > > I still think you should move this #ifdef inside
> > > > > > chrome::VersionInfo::GetChannel().
> > > > > 
> > > > > I disagree with this. Clients of this function should know explicitly
> that
> > > it
> > > > > does IO.
> > > > 
> > > > It would be better to just make it an async API and/or cache the value
on
> > > > startup, IMO.
> > > 
> > > Definitely agree, but Achuith said (and I agree) that that's work outside
of
> > > this CL. I just don't want clients implicitly doing IO on a thread on
which
> it
> > > shouldn't be done.
> > 
> > I don't think it's very good API to make all callers know about the side
> effects
> > of calling it.
> 
> (in a sense that they must make a scoped io allowance in their *code*, not
that
> the shouldn't be aware it might take a while or is slow)

ScopedAllowedIO is a hack to allow old designs to exist while they're
transitioned to message passing across the FILE thread(s). GetChannel should
probably take a closure to accept the result if it really needs to do IO, but it
doesn't. Making it easier to IO on other threads is not correct.

[achuithb, 2012-06-08 22:53:35]

On 2012/06/08 22:34:54, Dan Beam wrote:
> you committed before I LG'd and I was reviewing (not TBR'ing)
> 
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> File chrome/browser/web_resource/notification_promo.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10539045/diff/9001/chrome/browser/web_...
> chrome/browser/web_resource/notification_promo.cc:54:
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> On 2012/06/08 22:29:08, achuith.bhandarkar wrote:
> > On 2012/06/08 18:49:47, Dan Beam wrote:
> > > presubmit is angry at you for this ^, also
> > > 
> > > #if defined(OS_WIN)
> > >   base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > #endif
> > 
> > I'll add the OS_WIN #define. The ScopedAllowIO was not introduced by this
> patch,
> > but was moved from PromoResourceService.
> > 
> > I thought about this and it's going to be a decent amount of work to
eliminate
> > this ScopedAllowIO. channel is needed by GetPromoServerUrl(), which is
needed
> in
> > the ctor of WebResourceService. We'd need to refactor all that stuff to take
> > callbacks, and I'd rather not do that now.
> 
> I still think you should move this #ifdef inside
> chrome::VersionInfo::GetChannel().

I agree with Rob. Short term, ifdef OS_WIN will be in notification_promo.cc.
GetChannel should be called on a blocking thread, so moving the ScopedAllowIO
inside GetChannel to allow it to be called on a UI thread is wrong, not to
mention that I would have to override the presubmit script again.

Longer term, we'll refactor to make this async, but not for M21.

[Dan Beam, 2012-06-08 23:06:59]

On 2012/06/08 22:48:57, achuith.bhandarkar wrote:
> On 2012/06/08 22:34:54, Dan Beam wrote:
> > you committed before I LG'd and I was reviewing (not TBR'ing)
> > 
> 
> I got an LGTM from Aruslan, who's working on Clank (he's not a chromium
> committer yet), and he did take a look at it.
> 
> I did ping you and you said you were busy. There is a hard M21 deadline and
> unfortunately I can't afford to wait multiple days waiting on reviews for
> cleanup CLs that are blocking future work. Fwiw, my manager asked me to wait
for
> 24 hours for an lgtm and then escalate or TBR. In this case, the code was
> reviewed and I only needed an owner lgtm from you.

cleaning up dead code isn't needed for a feature or for a specific release.

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

On 2012/06/08 23:06:59, Dan Beam wrote:
> On 2012/06/08 22:48:57, achuith.bhandarkar wrote:
> > On 2012/06/08 22:34:54, Dan Beam wrote:
> > > you committed before I LG'd and I was reviewing (not TBR'ing)
> > > 
> > 
> > I got an LGTM from Aruslan, who's working on Clank (he's not a chromium
> > committer yet), and he did take a look at it.
> > 
> > I did ping you and you said you were busy. There is a hard M21 deadline and
> > unfortunately I can't afford to wait multiple days waiting on reviews for
> > cleanup CLs that are blocking future work. Fwiw, my manager asked me to wait
> for
> > 24 hours for an lgtm and then escalate or TBR. In this case, the code was
> > reviewed and I only needed an owner lgtm from you.
> 
> cleaning up dead code isn't needed for a feature or for a specific release.

True. It wasn't clear when you said you were busy, if you were going to be busy
for a day or for a week. If it was just a matter of waiting for another day,
that would not have been a problem. I should've asked.