Delete dead AppsPromo code

Delete dead AppsPromo code


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

[Gaurav, 2012-07-31 18:29:03]

Deleting the AppsPromo code. The related css / js / resource code was already
deleted in http://codereview.chromium.org/8344013

[Mihai Parparita -not on Chrome, 2012-07-31 18:41:28]

Should also be able to remove kForceAppsPromoVisible from chrome_switches.{cc|h}

https://chromiumcodereview.appspot.com/10827102/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (left):

https://chromiumcodereview.appspot.com/10827102/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:446:
dictionary->SetString("promoHeader", data.header);
This promo data is referenced in chrome/browser/resources/ntp4/apps_page.js,
that should be removed too.

There are also some resources there (e.g.
chrome/browser/resources/ntp4/images/app_promo_button.png) that need to be
removed.

[Gaurav, 2012-07-31 22:23:29]

Deleted the related css, js functions.
Also addressed comments

[Mihai Parparita -not on Chrome, 2012-07-31 22:58:16]

http://codereview.chromium.org/10827102/diff/6001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.css (right):

http://codereview.chromium.org/10827102/diff/6001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:105: .tile:only-of-type >
.app-contents > span {
Seems like these rule should be removed entirely (since the has-promo class will
no longer appear, so all of the code is unreacheable).

[Gaurav, 2012-07-31 23:22:43]

comments

[Mihai Parparita -not on Chrome, 2012-08-01 00:00:37]

LGTM

[commit-bot: I haz the power, 2012-08-01 00:24:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10827102/3037

[commit-bot: I haz the power, 2012-08-01 00:24:36]

Failed to apply patch for chrome/browser/extensions/extension_service.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_service.cc
Hunk #1 FAILED at 35.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_service.cc.rej

[commit-bot: I haz the power, 2012-08-01 21:31:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10827102/2004

[commit-bot: I haz the power, 2012-08-01 21:31:28]

Failed to apply patch for chrome/browser/extensions/extension_service.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_service.cc
Hunk #1 FAILED at 35.
Hunk #2 FAILED at 334.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_service.cc.rej

[commit-bot: I haz the power, 2012-08-01 23:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10827102/9003

[commit-bot: I haz the power, 2012-08-01 23:34:21]

Presubmit check for 10827102-9003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui/webui/ntp
    chrome/browser/prefs
    chrome/browser/policy
    chrome/browser/resources/ntp4

Presubmit checks took 2.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Gaurav, 2012-08-03 19:25:48]

@Nico Can you review the changes under

 chrome/browser/ui/webui/ntp
    chrome/browser/prefs
    chrome/browser/policy
    chrome/browser/resources/ntp4

Thanks.

[Nico, 2012-08-03 19:31:47]

lgtm

[commit-bot: I haz the power, 2012-08-03 19:35:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10827102/8004

[Dan Beam, 2012-08-03 19:40:29]

grv: next time consider asking more local owners to review, as the presubmit
just looks for the minimum number of acceptably local reviewers, I believe, so
though estade@ and I are both in c/b/ui/webui/ntp and c/b/resources/ntp4 it
recommended somebody in chrome/OWNERS instead (thakis@).

[Nico, 2012-08-03 19:44:58]

On 2012/08/03 19:40:29, Dan Beam wrote:
> grv: next time consider asking more local owners to review, as the presubmit
> just looks for the minimum number of acceptably local reviewers, I believe, so
> though estade@ and I are both in c/b/ui/webui/ntp and c/b/resources/ntp4 it
> recommended somebody in chrome/OWNERS instead (thakis@).

dbeam: To be fair, the higher-up OWNERS are supposed to say that too if they
deem a change non-trivial. This looked like it's just deleting code though (I
tried to check that the deleted css classes aren't used anywhere else), so I
figured it's ok to just let it through to improve review latency. Do you think I
should've asked for your review? Do you want to review everything below ntp4/ --
if not, what's the criterion I should use to decide what to hand on to you?

(tl;dr: my fault, not grvs. Sorry.)

[Dan Beam, 2012-08-03 19:56:34]

On 2012/08/03 19:44:58, Nico wrote:
> On 2012/08/03 19:40:29, Dan Beam wrote:
> > grv: next time consider asking more local owners to review, as the presubmit
> > just looks for the minimum number of acceptably local reviewers, I believe,
so
> > though estade@ and I are both in c/b/ui/webui/ntp and c/b/resources/ntp4 it
> > recommended somebody in chrome/OWNERS instead (thakis@).
> 
> dbeam: To be fair, the higher-up OWNERS are supposed to say that too if they
> deem a change non-trivial. This looked like it's just deleting code though (I
> tried to check that the deleted css classes aren't used anywhere else), so I
> figured it's ok to just let it through to improve review latency. Do you think
I
> should've asked for your review? Do you want to review everything below ntp4/
--
> if not, what's the criterion I should use to decide what to hand on to you?

I think it's surprisingly hard to correctly remove code you're not really
familiar with as there will likely be other bits that aren't apparent or future
plans that might not be known, etc. Additionally, the NTP doesn't have a ton of
automated tests, so generally having an OWNER look at the change / patch them in
and run a quick acceptability test (against the laundry list of features
probably only we know about) tends to be necessary to lower regression risk
(unfortunately speaking from past experience).

> (tl;dr: my fault, not grvs. Sorry.)

It's not a big deal, that's why we have watchlists, ;)

[Gaurav, 2012-08-03 20:15:44]

On 2012/08/03 19:56:34, Dan Beam wrote:
> On 2012/08/03 19:44:58, Nico wrote:
> > On 2012/08/03 19:40:29, Dan Beam wrote:
Thanks Nico and Dan.

I am new to the team, so I just looked at the owners list in chrome/ and sent
for review(as presubmit checks were nagging at me). 
Will add local owners henceforth.


> > > grv: next time consider asking more local owners to review, as the
presubmit
> > > just looks for the minimum number of acceptably local reviewers, I
believe,
> so
> > > though estade@ and I are both in c/b/ui/webui/ntp and c/b/resources/ntp4
it
> > > recommended somebody in chrome/OWNERS instead (thakis@).
> > 
> > dbeam: To be fair, the higher-up OWNERS are supposed to say that too if they
> > deem a change non-trivial. This looked like it's just deleting code though
(I
> > tried to check that the deleted css classes aren't used anywhere else), so I
> > figured it's ok to just let it through to improve review latency. Do you
think
> I
> > should've asked for your review? Do you want to review everything below
ntp4/
> --
> > if not, what's the criterion I should use to decide what to hand on to you?
> 
> I think it's surprisingly hard to correctly remove code you're not really
> familiar with as there will likely be other bits that aren't apparent or
future
> plans that might not be known, etc. Additionally, the NTP doesn't have a ton
of
> automated tests, so generally having an OWNER look at the change / patch them
in
> and run a quick acceptability test (against the laundry list of features
> probably only we know about) tends to be necessary to lower regression risk
> (unfortunately speaking from past experience).
> 
> > (tl;dr: my fault, not grvs. Sorry.)
> 
> It's not a big deal, that's why we have watchlists, ;)

[Mihai Parparita -not on Chrome, 2012-08-03 20:21:22]

On Fri, Aug 3, 2012 at 12:40 PM, <dbeam@chromium.org> wrote:

> grv: next time consider asking more local owners to review, as the
> presubmit
> just looks for the minimum number of acceptably local reviewers, I
> believe, so
> though estade@ and I are both in c/b/ui/webui/ntp and c/b/resources/ntp4
> it
> recommended somebody in chrome/OWNERS instead (thakis@).
>

Actually, it recommended chrome/OWNERS since Gaurav touched files in
chrome/common, and that doesn't have its own OWNERS file, so it picked up
the parent.

For the sake of full disclosure, I'm the one who told Gaurav to only ask
for chrome/ OWNERS approval, since it would encompass the 4 other distinct
directories that the script complained about.

Mihai

[commit-bot: I haz the power, 2012-08-03 20:51:36]

Change committed as 149919

[Evan Stade, 2012-08-03 21:02:25]

lgtm

[Joao da Silva, 2012-08-06 09:36:32]

On 2012/08/03 21:02:25, Evan Stade wrote:
> lgtm

This broke the policy pyauto tests, and the HideWebStorePromo policy must be
marked as deprecated. Please give the enterprise team a heads up when changing
policy/ stuff. Thanks!