Pipe shortcut app badge to shelf model [chromium/src : main]
Toni Barzic (Gerrit)
Toni Barzic uploaded patch set #2 to this change.
Pipe shortcut app badge to shelf model
Change-Id: I3ef60004c924e268c94a7e28e5967aa999282239
---
M ash/public/cpp/shelf_item.h
M chrome/browser/ash/app_list/app_service/app_service_app_icon_loader.cc
M chrome/browser/ash/app_list/app_service/app_service_promise_app_icon_loader.cc
M chrome/browser/ash/app_list/app_service/app_service_shortcut_icon_loader.cc
M chrome/browser/ash/app_list/app_service/app_service_shortcut_icon_loader.h
M chrome/browser/ash/app_list/arc/arc_app_unittest.cc
M chrome/browser/ash/app_list/search/arc/arc_app_shortcut_search_result.cc
M chrome/browser/ash/app_list/search/arc/arc_app_shortcut_search_result.h
M chrome/browser/ash/file_system_provider/notification_manager.cc
M chrome/browser/ash/file_system_provider/notification_manager.h
M chrome/browser/extensions/api/file_system/request_file_system_notification.cc
M chrome/browser/extensions/chrome_app_icon_loader.cc
M chrome/browser/extensions/chrome_app_icon_unittest.cc
M chrome/browser/notifications/extension_notifier_controller.cc
M chrome/browser/notifications/extension_notifier_controller.h
M chrome/browser/ui/app_icon_loader_delegate.h
M chrome/browser/ui/ash/shelf/arc_app_window.cc
M chrome/browser/ui/ash/shelf/arc_app_window.h
M chrome/browser/ui/ash/shelf/chrome_shelf_controller.cc
M chrome/browser/ui/ash/shelf/chrome_shelf_controller.h
M chrome/browser/ui/ash/shelf/crostini_app_window.cc
M chrome/browser/ui/views/arc_app_dialog_view.cc
M chrome/browser/ui/views/arc_data_removal_dialog_view.cc
23 files changed, 75 insertions(+), 36 deletions(-)
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Toni Barzic.
Toni Barzic uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Toni Barzic
23 files changed, 74 insertions(+), 36 deletions(-)
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Toni Barzic.
23 files changed, 81 insertions(+), 48 deletions(-)
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Toni Barzic.
Toni Barzic uploaded patch set #5 to this change.
Pipe shortcut app badge to shelf model
For app shortcuts, app items in the shelf should have a badge (the
badge will be the app icon of the app that hosts the app shortcut).
This updates AppIconLoaderDelegate::OnAppImageUpdated to pass the badge
image in addition to the app image. The badge image is currently set for
app shortcuts only.
BUG=b:306294395
Toni Barzic (Gerrit)
Attention is currently required from: Maggie Cai, Owen Zhang.
Toni Barzic would like Owen Zhang and Maggie Cai to review this change.
Owen Zhang (Gerrit)
Attention is currently required from: Maggie Cai, Toni Barzic.
1 comment:
Patchset:
LGTM, very helpful Toni, thanks!
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Maggie Cai (Gerrit)
Attention is currently required from: Toni Barzic.
1 comment:
File chrome/browser/ui/app_icon_loader_delegate.h:
Patch Set #5, Line 20:
OnAppImageUpdatedJust wondering if it make more sense to have a separate interface for badge image updated? It looks like this added argument is only used in one place bug we are changing the interface everywhere. can they be updated separately? In the backend it is possible that we have a updated badge icon but no updated shortcut icon
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Kevin Radtke (Gerrit)
Attention is currently required from: Toni Barzic.
Kevin Radtke uploaded patch set #6 to the change originally created by Toni Barzic.
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Kevin Radtke, Maggie Cai.
1 comment:
File chrome/browser/ui/app_icon_loader_delegate.h:
Patch Set #5, Line 20:
OnAppImageUpdated
Just wondering if it make more sense to have a separate interface for badge image updated? It looks […]
yeah, having a separate badge image updated interface is an interesting idea.
Though, with the current state of the code, sending icon updates separately seemed awkward given that they're generally created at the same time (and we probably want to badge the shortcut app icon from the start - I.e. badge seems to be a component of the overall app image, rather than a separate entity). And this avoids some edge cases where we could first paint the app badge, and then the main icon (depending on how the app icon loader behaves).
And one other advantage of returning main and badge image together would be that it may make developers think about whether their UI needs a badge (it just happens that for or non shelf view usages of AppIconLoader, it's used for app types that don't need badges).
Though, maybe if we untangle image generation and UI updates a bit more, I could see us having a separate badge icon loader (right now, both main and badge icon changes would be handled the same - shelf view just gets generic ShelfItemUpdated event, so we'd have some duplicate work with to separate IconUpdated interfaces).
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Maggie Cai (Gerrit)
Attention is currently required from: Kevin Radtke, Toni Barzic.
Patch set 6:Code-Review +1
1 comment:
File chrome/browser/ui/app_icon_loader_delegate.h:
Patch Set #5, Line 20:
OnAppImageUpdated
yeah, having a separate badge image updated interface is an interesting idea. […]
i see...emmm...then maybe we could change the badge_image to an optional value to indicate it doesn't have to be filled? but otherwise this LGTM
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Kevin Radtke, Toni Barzic.
Toni Barzic uploaded patch set #7 to this change.
23 files changed, 118 insertions(+), 65 deletions(-)
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Patch Set #5, Line 20:
OnAppImageUpdated
i see...emmm... […]
Done
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Ahmed Fakhry, Scott Violet.
Toni Barzic would like Scott Violet and Ahmed Fakhry to review this change.
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Ahmed Fakhry, Scott Violet.
1 comment:
Patchset:
Adding more owners:
sky for chrome/browser/ui/
afakhry for ash changes
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Ahmed Fakhry, Toni Barzic.
Patch set 7:Code-Review +1
Toni Barzic (Gerrit)
Attention is currently required from: Ahmed Fakhry, Giovanni Ortuno Urquidi.
Toni Barzic would like Giovanni Ortuno Urquidi to review this change.
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Ahmed Fakhry, Giovanni Ortuno Urquidi.
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Giovanni Ortuno Urquidi (Gerrit)
Attention is currently required from: Ahmed Fakhry, Toni Barzic.
Patch set 7:Code-Review +1
To view, visit change 4987448. To unsubscribe, or for help writing mail filters, visit settings.
Toni Barzic (Gerrit)
Attention is currently required from: Ahmed Fakhry.
Patch set 7:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Pipe shortcut app badge to shelf model
For app shortcuts, app items in the shelf should have a badge (the
badge will be the app icon of the app that hosts the app shortcut).
This updates AppIconLoaderDelegate::OnAppImageUpdated to pass the badge
image in addition to the app image. The badge image is currently set for
app shortcuts only.
BUG=b:306294395
Change-Id: I3ef60004c924e268c94a7e28e5967aa999282239
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4987448
Reviewed-by: Scott Violet <s...@chromium.org>
Reviewed-by: Maggie Cai <mx...@chromium.org>
Commit-Queue: Toni Barzic <tba...@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ort...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1218589}