Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2102433002:
Filter out ARC notifiers that are not shown in launcher. (Closed)
|
Created:
4 years, 5 months ago by hirono Modified:
4 years, 5 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, yoshiki Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFilter out Android system packages from Chrome OS notification settings.
In Android Settings application, there are no UI to disable
notifications for system packages.
The CL filter out system packages from Chrome OS notification settings
UI as well.
BUG=623436
TEST=manually tested on minnie
Committed: https://crrev.com/1f918658ca2a84d0124aaf98bd69e3755d9c61b2
Cr-Commit-Position: refs/heads/master@{#403761}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add system flag to package info. #Patch Set 3 : Filter out Android system packages from Chrome OS notification settings. #
Total comments: 4
Patch Set 4 : . #
Total comments: 2
Messages
Total messages: 33 (11 generated)
hirono@chromium.org changed reviewers: + dewittj@chromium.org
PTAL, thank you!
Description was changed from ========== Remove Android Settings from Chrome OS notification settings. Android Settings is hidden in launcher. We should hide it in notificaiton settings as well. BUG=623436 TEST=manually tested on minnie ========== to ========== Filter out applications that are not shown in launcher. The applications in launcher is filter by arc::ShouldShowInLauncher utility function. The CL filters out applications from Chrome OS notification setting UI by using the same utility function. BUG=623436 TEST=manually tested on minnie ==========
Description was changed from ========== Filter out applications that are not shown in launcher. The applications in launcher is filter by arc::ShouldShowInLauncher utility function. The CL filters out applications from Chrome OS notification setting UI by using the same utility function. BUG=623436 TEST=manually tested on minnie ========== to ========== Filter out ARC notifiers that are not shown in launcher. The applications in launcher is filter by arc::ShouldShowInLauncher utility function. The CL filters out applications from Chrome OS notification setting UI by using the same utility function. BUG=623436 TEST=manually tested on minnie ==========
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
lgtm https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... File chrome/browser/notifications/arc_application_notifier_source_chromeos.h (left): https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... chrome/browser/notifications/arc_application_notifier_source_chromeos.h:30: // TODO(hirono): Observe enabled flag change and notify it to message center. this change is unrelated to this?
https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:50: if (!app || !arc::ShouldShowInLauncher(app_id)) Can an app that's not in the launcher show notifications?
uekawa@google.com changed reviewers: + yoshiki@chromium.org
uekawa@google.com changed reviewers: + uekawa@google.com
+yoshiki https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:50: if (!app || !arc::ShouldShowInLauncher(app_id)) On 2016/06/27 16:17:49, dewittj wrote: > Can an app that's not in the launcher show notifications? There are launchable activities and other activities, app_list->GetApp() seems to be obtaining the list of launchable activities. hmmm.. is this right?
https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:50: if (!app || !arc::ShouldShowInLauncher(app_id)) On 2016/06/27 23:15:09, uekawa- wrote: > On 2016/06/27 16:17:49, dewittj wrote: > > Can an app that's not in the launcher show notifications? > > There are launchable activities and other activities, app_list->GetApp() seems > to be obtaining the list of launchable activities. > > hmmm.. is this right? I think there is two points here. First the notifications enabled flag is set for packages in Android side. So if the package has at least one launchable acitvity, users can control the notifications flag for the package. Users cannot block notifications from packages that have no launchable activity (e.g. ContentProvider that shows notifications) but I guess the number of such package is small. Second arc::ShouldShowInLauncher filter out activities that are launchable but we would not like to show in Chrome launcher. Currently it hides only Android Settings application. Android UI does not allow to block notifications from Settings app since it's system app. So I'd like to remove Android Settings application from notifier list. Now I have two plans here. a. Use arc::ShouldShowInLauncher to filter out Settings applications since arc::ShouldShowInLauncher is almost used to filter out launch-able system applications. b. Add is_system flag to application info and filter out all system applications from notifier list. WDYT?
On 2016/06/28 02:20:32, hirono wrote: > https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... > File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc > (right): > > https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... > chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:50: if > (!app || !arc::ShouldShowInLauncher(app_id)) > On 2016/06/27 23:15:09, uekawa- wrote: > > On 2016/06/27 16:17:49, dewittj wrote: > > > Can an app that's not in the launcher show notifications? > > > > There are launchable activities and other activities, app_list->GetApp() > seems > > to be obtaining the list of launchable activities. > > > > hmmm.. is this right? > > I think there is two points here. > > First the notifications enabled flag is set for packages in Android side. So if > the package has at least one launchable acitvity, users can control the > notifications flag for the package. Users cannot block notifications from > packages that have no launchable activity (e.g. ContentProvider that shows > notifications) but I guess the number of such package is small. > > Second arc::ShouldShowInLauncher filter out activities that are launchable but > we would not like to show in Chrome launcher. Currently it hides only Android > Settings application. > > Android UI does not allow to block notifications from Settings app since it's > system app. So I'd like to remove Android Settings application from notifier > list. Now I have two plans here. > > a. Use arc::ShouldShowInLauncher to filter out Settings applications since > arc::ShouldShowInLauncher is almost used to filter out launch-able system > applications. > b. Add is_system flag to application info and filter out all system applications > from notifier list. > > WDYT? ping, thank you!
https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/1/chrome/browser/notification... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:50: if (!app || !arc::ShouldShowInLauncher(app_id)) On 2016/06/28 02:20:31, hirono wrote: > On 2016/06/27 23:15:09, uekawa- wrote: > > On 2016/06/27 16:17:49, dewittj wrote: > > > Can an app that's not in the launcher show notifications? > > > > There are launchable activities and other activities, app_list->GetApp() > seems > > to be obtaining the list of launchable activities. > > > > hmmm.. is this right? > > I think there is two points here. > > First the notifications enabled flag is set for packages in Android side. So if > the package has at least one launchable acitvity, users can control the > notifications flag for the package. Users cannot block notifications from > packages that have no launchable activity (e.g. ContentProvider that shows > notifications) but I guess the number of such package is small. > > Second arc::ShouldShowInLauncher filter out activities that are launchable but > we would not like to show in Chrome launcher. Currently it hides only Android > Settings application. > > Android UI does not allow to block notifications from Settings app since it's > system app. So I'd like to remove Android Settings application from notifier > list. Now I have two plans here. > > a. Use arc::ShouldShowInLauncher to filter out Settings applications since > arc::ShouldShowInLauncher is almost used to filter out launch-able system > applications. > b. Add is_system flag to application info and filter out all system applications > from notifier list. > > WDYT? I am not sure if you are waiting for me or not. If you ware waiting for me, at this layer I would love to see a function arc::UserCanControlNotifierSettings(app_id) or something whose purpose is obvious. Then at the ARC layer you can implement it however you like.
I updated the CL so that it refers system flag to filter out Android packages from Chrome OS notifications UI. Could you take another look? Thank you! @yoshiki, @uekawa - Overall design. @dewittj - c/b/notifications @xiyuan - c/b/ui/app_list @dcheng - components/arc/common/app.mojom Thank you!
hirono@chromium.org changed reviewers: + dcheng@chromium.org, xiyuan@chromium.org
Forgot to add new owners. > I updated the CL so that it refers system flag to filter out Android packages > from Chrome OS notifications UI. > Could you take another look? Thank you! > > @yoshiki, @uekawa - Overall design. > @dewittj - c/b/notifications > @xiyuan - c/b/ui/app_list > @dcheng - components/arc/common/app.mojom > > Thank you!
Is the CL description still correct? The CL seems be doing "Filter out ARC notifiers for system packages" rather than "not shown in launcher". https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:57: continue; nit: wrong indent ?
this lgtm https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:49: if (!app) nit: combine all these if statements
mojom lgtm but I think the CL description needs to be updated
Description was changed from ========== Filter out ARC notifiers that are not shown in launcher. The applications in launcher is filter by arc::ShouldShowInLauncher utility function. The CL filters out applications from Chrome OS notification setting UI by using the same utility function. BUG=623436 TEST=manually tested on minnie ========== to ========== Filter out Android system packages from Chrome OS notification settings. In Android Settings application, there are no UI to disable notifications for system packages. The CL filter out system packages from Chrome OS notification settings UI as well. BUG=623436 TEST=manually tested on minnie ==========
Updated the CL description. Thank you! https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/arc_application_notifier_source_chromeos.cc (right): https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:49: if (!app) On 2016/06/30 17:55:27, dewittj wrote: > nit: combine all these if statements Done. https://codereview.chromium.org/2102433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/arc_application_notifier_source_chromeos.cc:57: continue; On 2016/06/30 14:43:03, xiyuan wrote: > nit: wrong indent ? Done.
lgtm
https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:28: [MinVersion=11] bool system; // true if package is system package. do you have a ARC side change?
https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:28: [MinVersion=11] bool system; // true if package is system package. On 2016/07/04 02:01:26, uekawa- wrote: > do you have a ARC side change? Yes, uploaded ag/1197580.
On 2016/07/04 06:53:57, hirono wrote: > https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... > File components/arc/common/app.mojom (right): > > https://codereview.chromium.org/2102433002/diff/60001/components/arc/common/a... > components/arc/common/app.mojom:28: [MinVersion=11] bool system; // true if > package is system package. > On 2016/07/04 02:01:26, uekawa- wrote: > > do you have a ARC side change? > > Yes, uploaded ag/1197580. @yoshiki - Can I submit the change? The CL got LGTM from file owners.
lgtm
The CQ bit was checked by hirono@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from uekawa@chromium.org, dcheng@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2102433002/#ps60001 (title: ".")
Thank you!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Filter out Android system packages from Chrome OS notification settings. In Android Settings application, there are no UI to disable notifications for system packages. The CL filter out system packages from Chrome OS notification settings UI as well. BUG=623436 TEST=manually tested on minnie ========== to ========== Filter out Android system packages from Chrome OS notification settings. In Android Settings application, there are no UI to disable notifications for system packages. The CL filter out system packages from Chrome OS notification settings UI as well. BUG=623436 TEST=manually tested on minnie Committed: https://crrev.com/1f918658ca2a84d0124aaf98bd69e3755d9c61b2 Cr-Commit-Position: refs/heads/master@{#403761} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1f918658ca2a84d0124aaf98bd69e3755d9c61b2 Cr-Commit-Position: refs/heads/master@{#403761} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
