New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 13
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Task



Sign in to add a comment
link

Issue 910767: Remove carrier mobile data promo notifications

Reported by tbarzic@chromium.org, Nov 30 Project Member

Issue description

Ash currently supports notification for mobile data carrier promotions, which are shown when the user first connects to a specific cellular network
(see chrome/browser/ui/ash/network/data_promo_notification.h).

The promotions are fetched from mobile_config.json files installed by chromeos-assets package - and yes, all the promotions contained in the file have expired some time mid 2012  - we should remove support for this notification.
(and probably mobile_config.json all together, it doesn't seem to be used for anything else, at least not in Chrome)
 

Comment 1 by bugdroid1@chromium.org, Dec 10

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e29d459ca52cba12ffe2026ea83cd50f06c869fc

commit e29d459ca52cba12ffe2026ea83cd50f06c869fc
Author: Tony de Luna <tonydeluna@chromium.org>
Date: Mon Dec 10 19:39:53 2018

Remove support for mobile data promos

Removes logic for displaying mobile data promos. We haven't had mobile
data promos since 2012.

Refactors notification displaying logic, renames variables and method
names to avoid the promo concept.

I'll follow up with a cl that renames this class and all references
to it.

BUG= 910767 

Change-Id: I4deda2407b268729405af734241b8f36de2d2de9
Reviewed-on: https://chromium-review.googlesource.com/c/1359825
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615204}
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/chromeos/login/ui/login_display_host_common.cc
[delete] https://crrev.com/7c69b5099532dcbf5269c3016d20bd3c43c607a4/chrome/browser/chromeos/mobile_config.cc
[delete] https://crrev.com/7c69b5099532dcbf5269c3016d20bd3c43c607a4/chrome/browser/chromeos/mobile_config.h
[delete] https://crrev.com/7c69b5099532dcbf5269c3016d20bd3c43c607a4/chrome/browser/chromeos/mobile_config_unittest.cc
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/ui/ash/network/data_promo_notification.cc
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/browser/ui/ash/network/data_promo_notification.h
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/common/pref_names.cc
[modify] https://crrev.com/e29d459ca52cba12ffe2026ea83cd50f06c869fc/chrome/common/pref_names.h

Comment 2 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4ae7da177ada33215ecb61969ade5a6570aa349

commit a4ae7da177ada33215ecb61969ade5a6570aa349
Author: Tony de Luna <tonydeluna@chromium.org>
Date: Wed Dec 12 22:13:20 2018

Rename DataPromoNotification to MobileDataNotifications

Notifications for displaying data promotions are no longer supported.

Class is now in charge of displaying mobile data warnings and prompts
user to install data saver extension.

BUG= 910767 

Change-Id: I74c45001158194788fb335be83e040b95e3271be
Reviewed-on: https://chromium-review.googlesource.com/c/1362211
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616076}
[modify] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
[rename] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/ash/network/mobile_data_notifications.cc
[rename] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/ash/network/mobile_data_notifications.h
[rename] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/browser/ui/ash/network/mobile_data_notifications_unittest.cc
[modify] https://crrev.com/a4ae7da177ada33215ecb61969ade5a6570aa349/chrome/test/BUILD.gn

Comment 3 by tonydeluna@chromium.org, Dec 14

Owner: tonydeluna@chromium.org
Status: Started (was: Available)

Comment 4 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49d89d55ff609a1adc9511866e1b2e0358cd391f

commit 49d89d55ff609a1adc9511866e1b2e0358cd391f
Author: Tony de Luna <tonydeluna@chromium.org>
Date: Fri Dec 14 19:17:34 2018

cros: Remove data saver extension notification

Stop showing chromeos users a notification prompting them to install a
data saver extension the first time they connect with a cellular
connection. Extension was recently removed.

Remove flag 'enable-datasaver-prompt' and deprecate
prefs::kDataSaverPromptsShown.

Add more tests to validate that notifications are only shown once.

Bug:  914470 ,  910767 
Change-Id: I0c7799d9a57fcb207fcea946a4454b4edc466ef4
Reviewed-on: https://chromium-review.googlesource.com/c/1374581
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616779}
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/about_flags.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/ui/ash/network/mobile_data_notifications.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/ui/ash/network/mobile_data_notifications.h
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chrome/browser/ui/ash/network/mobile_data_notifications_unittest.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chromeos/chromeos_switches.cc
[modify] https://crrev.com/49d89d55ff609a1adc9511866e1b2e0358cd391f/chromeos/chromeos_switches.h

Comment 5 by bugdroid1@chromium.org, Dec 18

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/feec5c2fb91ff5d802ace328c18d2700720fc86e

commit feec5c2fb91ff5d802ace328c18d2700720fc86e
Author: Tony de Luna <tonydeluna@chromium.org>
Date: Tue Dec 18 01:05:08 2018

Display mobile data notifications as system notifications

Bug:  910767 
Change-Id: Icb5adce6672a9a235df2b8b0c084c5ef9519a00a
Reviewed-on: https://chromium-review.googlesource.com/c/1380795
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617321}
[modify] https://crrev.com/feec5c2fb91ff5d802ace328c18d2700720fc86e/chrome/browser/ui/ash/network/mobile_data_notifications.cc
[modify] https://crrev.com/feec5c2fb91ff5d802ace328c18d2700720fc86e/chrome/browser/ui/ash/network/mobile_data_notifications_unittest.cc

Comment 6 by tonydeluna@chromium.org, Jan 8

As part of this code we're showing a warning notification to the user the first time they've used cellular data on their device.

Once shown, the notification is there until the user closes it. 


Should we be hiding this notification if the user switches back to WiFi/Ethernet? 

Maybe we can let it be shown for 10 seconds and hide it?

Comment 7 by bugdroid1@chromium.org, Jan 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c07af6a5606da8c5c68df653543d8e8290d75d3b

commit c07af6a5606da8c5c68df653543d8e8290d75d3b
Author: Tony de Luna <tonydeluna@chromium.org>
Date: Mon Jan 14 23:34:03 2019

mobile notifications: update observers

Stop subscribing to all network property updates.

Subscribe to default network changes to show a notification when
cellular becomes the default network.

Subscribe to session updates to show the notification if cellular
is the default network after a session change.

We avoid showing the notification if there are other network
connections pending. Subscribe to network connections changes to make
sure we don't accidentally miss legitimate updates.

Bug:  910767 
Change-Id: I189b16cb5fc19428c13848e7eb193952ca6655ac
Reviewed-on: https://chromium-review.googlesource.com/c/1399428
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622638}
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/chrome/browser/ui/ash/network/mobile_data_notifications.cc
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/chrome/browser/ui/ash/network/mobile_data_notifications.h
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/chrome/browser/ui/ash/network/mobile_data_notifications_unittest.cc
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/components/user_manager/user_manager_base.cc
[modify] https://crrev.com/c07af6a5606da8c5c68df653543d8e8290d75d3b/components/user_manager/user_manager_base.h

Comment 8 by jessejames@chromium.org, Feb 8

Labels: ChromeOS-LTE
Just checking in here, but what work remains on this? Thanks!

Comment 9 by khorimoto@chromium.org, Feb 13

Status: Fixed (was: Started)

Sign in to add a comment