|
|
Created:
7 years, 9 months ago by jeremya Modified:
7 years, 9 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 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
Messages
Total messages: 16 (0 generated)
Still kinda WIP in that I haven't manually tested this yet. But here it is :)
I think this is fine. Achuith: you?
https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... File chrome/browser/web_resource/notification_promo.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... chrome/browser/web_resource/notification_promo.cc:402: &is_app_launcher_promo)) nit: maybe line this up with the "is_app_launcher_promo" of the previous line? https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... chrome/browser/web_resource/notification_promo.cc:408: if (IsAppLauncherPromo() && Could you move this logic to IsAppLauncherPromo, rename IsAppLauncherPromo to something like CheckAppLauncher, and have it return a boolean. Then change the condition below to call && CheckAppLauncher. This would be consistent with how this function works. Find me on IM if this is not clear. https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/p... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/p... chrome/browser/web_resource/promo_resource_service_unittest.cc:338: void TestCanShow(bool should_show) { Let's eliminate this function and inline it below.
Could you please flesh out the description of this CL a little more? Perhaps explaining where this new key resides (the promo payload) and what it's useful for?
https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... File chrome/browser/web_resource/notification_promo.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... chrome/browser/web_resource/notification_promo.cc:402: &is_app_launcher_promo)) On 2013/03/18 19:24:08, achuith.bhandarkar wrote: > nit: maybe line this up with the "is_app_launcher_promo" of the previous line? Done. https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/n... chrome/browser/web_resource/notification_promo.cc:408: if (IsAppLauncherPromo() && On 2013/03/18 19:24:08, achuith.bhandarkar wrote: > Could you move this logic to IsAppLauncherPromo, rename IsAppLauncherPromo to > something like CheckAppLauncher, and have it return a boolean. Then change the > condition below to call && CheckAppLauncher. This would be consistent with how > this function works. Find me on IM if this is not clear. Done. https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/p... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://codereview.chromium.org/12601014/diff/1/chrome/browser/web_resource/p... chrome/browser/web_resource/promo_resource_service_unittest.cc:338: void TestCanShow(bool should_show) { On 2013/03/18 19:24:08, achuith.bhandarkar wrote: > Let's eliminate this function and inline it below. I did it this way because the notification_promo_ object is private, but I've just exposed it.
Thanks for taking care of the comments. lgtm.
On 2013/03/19 00:21:36, achuith.bhandarkar wrote: > Thanks for taking care of the comments. > > lgtm. Please don't land without a manual test (you should host your own promo and use --promo-server-url).
Tested it locally (after much confusion: i had to set the 'handler' in example_promo.go to 'ntp_notification_promo' and set the MaxIncrement = the number of groups in order to get it to show up). Seems to work fine.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12601014/8001
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2013/03/19 06:17:46, jeremya wrote: > Tested it locally (after much confusion: i had to set the 'handler' in > example_promo.go to 'ntp_notification_promo' and set the MaxIncrement = the > number of groups in order to get it to show up). Seems to work fine. Ah ha! Thanks, I'll fix that so it works in the future.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12601014/8001
Message was sent while issue was closed.
Change committed as 189193
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); 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?
Message was sent while issue was closed.
jeremya - can you take care of this please? 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); 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.
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. |