Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(435)

Issue 12601014: Add an is_app_launcher_promo key to NotificationPromo. (Closed)

Created:
7 years, 9 months ago by jeremya
Modified:
7 years, 9 months ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Visibility:
Public.

Description

Add an is_app_launcher_promo key to NotificationPromo. If the "is_app_launcher_promo" key is set in the promo payload, the promo won't be shown if the user has enabled the app launcher. Which is useful if the promo is asking the user to enable the app launcher :) R=rsesek@chromium.org BUG=178936 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189193

Patch Set 1 #

Total comments: 6

Patch Set 2 : comments #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -1 line) Patch
M chrome/browser/web_resource/notification_promo.h View 1 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/web_resource/notification_promo.cc View 1 2 chunks +11 lines, -0 lines 3 comments Download
M chrome/browser/web_resource/promo_resource_service_unittest.cc View 1 3 chunks +49 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
jeremya
Still kinda WIP in that I haven't manually tested this yet. But here it is ...
7 years, 9 months ago (2013-03-15 02:02:55 UTC) #1
Robert Sesek
I think this is fine. Achuith: you?
7 years, 9 months ago (2013-03-18 19:08:10 UTC) #2
achuithb
https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/notification_promo.cc File chrome/browser/web_resource/notification_promo.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/notification_promo.cc#newcode402 chrome/browser/web_resource/notification_promo.cc:402: &is_app_launcher_promo)) nit: maybe line this up with the "is_app_launcher_promo" ...
7 years, 9 months ago (2013-03-18 19:24:08 UTC) #3
achuithb
Could you please flesh out the description of this CL a little more? Perhaps explaining ...
7 years, 9 months ago (2013-03-18 19:25:47 UTC) #4
jeremya
https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/notification_promo.cc File chrome/browser/web_resource/notification_promo.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/notification_promo.cc#newcode402 chrome/browser/web_resource/notification_promo.cc:402: &is_app_launcher_promo)) On 2013/03/18 19:24:08, achuith.bhandarkar wrote: > nit: maybe ...
7 years, 9 months ago (2013-03-19 00:14:22 UTC) #5
achuithb
Thanks for taking care of the comments. lgtm.
7 years, 9 months ago (2013-03-19 00:21:36 UTC) #6
achuithb
On 2013/03/19 00:21:36, achuith.bhandarkar wrote: > Thanks for taking care of the comments. > > ...
7 years, 9 months ago (2013-03-19 00:22:46 UTC) #7
jeremya
Tested it locally (after much confusion: i had to set the 'handler' in example_promo.go to ...
7 years, 9 months ago (2013-03-19 06:17:46 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12601014/8001
7 years, 9 months ago (2013-03-19 06:19:03 UTC) #9
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=110316
7 years, 9 months ago (2013-03-19 07:54:04 UTC) #10
Robert Sesek
On 2013/03/19 06:17:46, jeremya wrote: > Tested it locally (after much confusion: i had to ...
7 years, 9 months ago (2013-03-19 14:32:16 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12601014/8001
7 years, 9 months ago (2013-03-19 23:11:49 UTC) #12
commit-bot: I haz the power
Change committed as 189193
7 years, 9 months ago (2013-03-20 01:42:18 UTC) #13
msarda
https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_resource/notification_promo.cc File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_resource/notification_promo.cc#newcode405 chrome/browser/web_resource/notification_promo.cc:405: !prefs_->GetBoolean(apps::prefs::kAppLauncherIsEnabled); The code in chrome/browser/web_resource/notification_promo.cc should not depend directly ...
7 years, 9 months ago (2013-03-22 15:02:30 UTC) #14
achuithb
jeremya - can you take care of this please? https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_resource/notification_promo.cc File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_resource/notification_promo.cc#newcode405 chrome/browser/web_resource/notification_promo.cc:405: ...
7 years, 9 months ago (2013-03-22 18:44:40 UTC) #15
msarda
7 years, 9 months ago (2013-03-25 09:12:41 UTC) #16
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/12601014/diff/8001/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:405:
!prefs_->GetBoolean(apps::prefs::kAppLauncherIsEnabled);
This method should return "true" on iOS based on its description: "Returns false
if this promo should not be displayed because it is a promo for the app
launcher, and the user has already enabled the app launcher."

I think there is a try job for iOS, but the iOS code that depends on
notification_promo.cc has not yet been upstreamed. We observed this problem
during a downstream merge.

On 2013/03/22 18:44:40, achuith.bhandarkar wrote:
> On 2013/03/22 15:02:30, msarda wrote:
> > The code in chrome/browser/web_resource/notification_promo.cc should not
> depend
> > directly on apps. Not all platforms build the apps/... code (iOS does not
> build
> > the apps/ code).
> > 
> > This piece of code does not build on iOS as
apps::prefs::kAppLauncherIsEnabled
> > is defined in apps/pref_names.h and this file is not build on iOS.
> > I see 2 options here:
> > 1. Move apps::prefs::kAppLauncherIsEnabled to another part of the project
that
> > is build on all platforms (this is a pref, maybe chrome/common/pref_names.h
> > would be a good location).
> > 2. Add extra forking by ifdef-ing out the body of this function on iOS.
> > 
> > What would be the right solution here?
> 
> I think we can just ifdef this function so CheckAppLauncher returns false on
> iOS. What is the right ifdef? Is there an ifdef for platforms that build apps?
> Does this build on android? I neglected to bring this up, but we should run a
> try job against iOS and android as well.

Powered by Google App Engine
This is Rietveld 408576698