Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894443002:
arc: Set custom icon in shelf for ARC apps. (Closed)
Descriptionarc: Set custom icon in shelf for ARC apps.
This handles setTaskDescription requests which provides optional icon
for the task. This CL receives icon from ARC and associates it with
ARC app window. ARC app window contoller receives updates and update
controller item accordingly. If no custom icon set then default app
icon is used.
Preview:
https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qM3FTYWVJdl9FMDA/view?usp=sharing
TEST=Manually + unit_test
BUG=723742
Review-Url: https://codereview.chromium.org/2894443002
Cr-Commit-Position: refs/heads/master@{#474663}
Committed: https://chromium.googlesource.com/chromium/src/+/f4821feb1dfd4ad2b98ffc4be0c7deae02fa7e82
Patch Set 1 #Patch Set 2 : Prevent Play Store overwriting #Patch Set 3 : mojo based instead of wayland #Patch Set 4 : cleanup #
Total comments: 119
Patch Set 5 : comments addressed #
Total comments: 10
Patch Set 6 : comments addressed #
Total comments: 8
Patch Set 7 : nits + TODO #
Total comments: 10
Patch Set 8 : add icon size constrain #Patch Set 9 : add label check #Patch Set 10 : rebase #Messages
Total messages: 44 (12 generated)
khmel@chromium.org changed reviewers: + reveman@chromium.org
Hi David, PTAL Thanks!
have we consider using mojo for this instead? this seems closer to the launcher functionality than compositing and window management
On 2017/05/17 20:32:55, reveman wrote: > have we consider using mojo for this instead? this seems closer to the launcher > functionality than compositing and window management Yes, I considered. There are cons and pros We already sending label from task description via wayland, It is logically to have everything in one place. Passing icon from ActivityManager to AppLauncherService needs extra ARC specific coding in framework/base. From other side, yes, we can pass icon directly to app_window_launcher_controller and this could be simpler. Probably it is better to pass label + icon via mojo together. So, I don't have strong preference here. What do you prefer and have inclination?
Decoding a png from the client is not something we can support in a secure way as image decoders are easy to exploit with custom images. If we go the wayland route then this icon image needs to be shared in the form a surface. I think I'd be in favor of using mojo for title and icon as that's more in line with wayland usage on desktop linux. Title and icons are part of the launcher system there. However, as we have the title API already I'm fine adding icon API too if that makes things easier short term.
On 2017/05/17 21:01:11, reveman wrote: > Decoding a png from the client is not something we can support in a secure way > as image decoders are easy to exploit with custom images. If we go the wayland > route then this icon image needs to be shared in the form a surface. > > I think I'd be in favor of using mojo for title and icon as that's more in line > with wayland usage on desktop linux. Title and icons are part of the launcher > system there. However, as we have the title API already I'm fine adding icon API > too if that makes things easier short term. Thanks David, I will refactor this solution to mojom.
khmel@chromium.org changed reviewers: + lhchavez@chromium.org, msw@chromium.org, xiyuan@chromium.org
Hi, PTAL relevant code. Thank you! David, I set you as reviewer initially because it was wayland based. As we agreed it is better to use mojo for this task. Please feel free to review this CL or ignore it once wayland part is discarded.
https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:30: if (GetLastActiveWindow() == app_window) As mentioned in the other comment, it might be better to fold this into OnActiveWindowChanged impl. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:69: virtual void OnActiveWindowChanged(ui::BaseWindow* active_window) {} The name is confusing. I thought it meant something like |last_active_window_| is changed. But it is not. It is more like the data about the active window (such as launcher icon for the window) is changed. How about call it UpdateLauncherIcon? The "active" part is hard to enforce on the call sites. It is probably better to let call sites to call with whatever window and we do the active window filtering inside the function impl. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:26: class CustomIconSource : public gfx::ImageSkiaSource { Can we get rid of this class and do something like the following ? image_skia_ = gfx::ImageSkiaOperations::CreateResizedImage( gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f), gfx::Size(extension_misc::EXTENSION_ICON_SMALL, extension_misc::EXTENSION_ICON_SMALL)); Think that should give us the same resized icon. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:74: controller_ = controller; Do we need to ensure that the launcher icon is updated in |controller_|? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:205: controller()->OnWindowChanged(this); Do we need to worry about |controller_| is nullptr? ResetIcon() does not seems to be guaranteed to have a controller. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:212: controller()->OnWindowChanged(this); same here. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:26: class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest { Is this done by "git cl format"? If not, might be nice to put each parent class in its own line. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:108: ArcAppWindowLauncherController* owner_; nit: ArcAppWindowLauncherController* const owner_; https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:109: void ArcAppWindowLauncherItemController::OnActiveWindowChanged( As mentioned elsewhere, this can be merged with OnWindowChanged as UpdateLauncherIcon.
https://codereview.chromium.org/2894443002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2894443002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:134: array<uint8> icon_png_data); Sending malicious image data to chrome is a well known attack vector. This is why we do all image decoding in sandboxed renderer processes for the web. How are we protecting against these type of attacks in this case? Can apps control the png data sent to Chrome? That a compromised Android container can use this is bad enough and if apps can control this then I think that's really bad. Please get a security review before you land this change.
khmel@chromium.org changed reviewers: + nasko@chromium.org
Thank you for comments. I updated the code. + nasko@ for mojom PTAL https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:30: if (GetLastActiveWindow() == app_window) On 2017/05/18 16:22:48, xiyuan wrote: > As mentioned in the other comment, it might be better to fold this into > OnActiveWindowChanged impl. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:69: virtual void OnActiveWindowChanged(ui::BaseWindow* active_window) {} On 2017/05/18 16:22:48, xiyuan wrote: > The name is confusing. I thought it meant something like |last_active_window_| > is changed. But it is not. It is more like the data about the active window > (such as launcher icon for the window) is changed. > > How about call it UpdateLauncherIcon? > > The "active" part is hard to enforce on the call sites. It is probably better to > let call sites to call with whatever window and we do the active window > filtering inside the function impl. Done. Thanks for offline chat. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:26: class CustomIconSource : public gfx::ImageSkiaSource { On 2017/05/18 16:22:49, xiyuan wrote: > Can we get rid of this class and do something like the following ? > > image_skia_ = gfx::ImageSkiaOperations::CreateResizedImage( > gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f), > gfx::Size(extension_misc::EXTENSION_ICON_SMALL, > extension_misc::EXTENSION_ICON_SMALL)); > > Think that should give us the same resized icon. Yes, if required scale is missing then 1.0 is taken and scaled. Done https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:74: controller_ = controller; On 2017/05/18 16:22:49, xiyuan wrote: > Do we need to ensure that the launcher icon is updated in |controller_|? Practically no, because AddWindow is called the same time as controller set. However it may make sense to add here call as well. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:205: controller()->OnWindowChanged(this); On 2017/05/18 16:22:48, xiyuan wrote: > Do we need to worry about |controller_| is nullptr? ResetIcon() does not seems > to be guaranteed to have a controller. Yes, it can happen, especially in multi-profile case. Done https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:212: controller()->OnWindowChanged(this); On 2017/05/18 16:22:48, xiyuan wrote: > same here. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:26: class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest { On 2017/05/18 16:22:49, xiyuan wrote: > Is this done by "git cl format"? If not, might be nice to put each parent class > in its own line. Hmm, I always put this to separate line. So this is "git cl format" trick. I validated once again and yes, formatter decides that this is better. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:108: ArcAppWindowLauncherController* owner_; On 2017/05/18 16:22:49, xiyuan wrote: > nit: ArcAppWindowLauncherController* const owner_; Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:109: void ArcAppWindowLauncherItemController::OnActiveWindowChanged( On 2017/05/18 16:22:49, xiyuan wrote: > As mentioned elsewhere, this can be merged with OnWindowChanged as > UpdateLauncherIcon. Done. https://codereview.chromium.org/2894443002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2894443002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:134: array<uint8> icon_png_data); On 2017/05/18 16:46:03, reveman wrote: > Sending malicious image data to chrome is a well known attack vector. This is > why we do all image decoding in sandboxed renderer processes for the web. How > are we protecting against these type of attacks in this case? Can apps control > the png data sent to Chrome? That a compromised Android container can use this > is bad enough and if apps can control this then I think that's really bad. > > Please get a security review before you land this change. We have several places already in this mojo to send icon data, for app and for shortcuts. Png data is compressed by ArcAppLauncherService from bitmap, the same as for app and shorcuts. Any icon sent via this mojo interface is processed via ImageDecoder::Start which is "sandboxed" for security reason. So this call does not add any new to security. Anyway mojo files are reviewed by special persons.
lgtm Thanks. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:26: class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest { On 2017/05/18 17:29:34, khmel wrote: > On 2017/05/18 16:22:49, xiyuan wrote: > > Is this done by "git cl format"? If not, might be nice to put each parent > class > > in its own line. > > Hmm, I always put this to separate line. So this is "git cl format" > trick. I validated once again and yes, formatter decides that this is better. Acknowledged.
components/arc lgtm with nits https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:127: return windows_.empty() ? nullptr : windows_.front(); nit: if (windows_.empty()) return nullptr return windows_.front(); https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:89: if (!arc_app_window || arc_app_window->image_skia().isNull()) { nit: use the guard clause pattern: if (!arc_app_window || arc_app_window->image_skia().isNull()) { if (!image_set_by_controller()) return; ... return; } owner_->... https://codereview.chromium.org/2894443002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2894443002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:131: // Sends task description. nit: Sends task label and icon. https://codereview.chromium.org/2894443002/diff/80001/components/arc/test/fak... File components/arc/test/fake_app_instance.h (right): https://codereview.chromium.org/2894443002/diff/80001/components/arc/test/fak... components/arc/test/fake_app_instance.h:126: const std::string& icon_png_data); nit: icon_png_data_as_string (for consistency with the other functions).
https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1066: void ArcAppListPrefs::OnTaskSetDescription( ditto nit: order to match interface decl https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:133: virtual void OnTaskSetDescription( nit: consider OnTaskDescriptionChanged, OnTaskDescriptionUpdated or similar. (or Changing/Updating if it has yet to actually change) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:299: void OnTaskSetDescription(int32_t task_id, nit: order to match interface decl. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:82: // Returns last active window in the controller or top window. nit: s/top/first/? (to avoid confusion with MRU, z-order, or placement) aside (for another cl): should something actually track MRU windows (maybe using MruWindowTracker and checking against |windows_|) to avoid just returning 'any' window if the |last_active_window_| is removed? Seems like we could maybe even get rid of |last_active_window_| in that case. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:83: ui::BaseWindow* GetLastActiveWindow(); nit: make this private if it's not used outside this class. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:21: // Set to true for unit tests to avoid IPC icon decoding. nit: explain why that's useful. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:22: bool disable_safe_icon_decoding = false; nit: append _for_testing to the identifier itself https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:89: if (title.length()) nit !empty? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:90: GetNativeWindow()->SetTitle(base::string16(base::UTF8ToUTF16(title))); nit: base::string16 is probably not needed (adds a copy) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:93: ResetIcon(); Can we just continue using the current icon until a new one is successfully decoded? It would be nice to avoid unnecessary work/code and avoid blank icons flashing during decode, etc. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:111: return widget_->IsActive() && owner_->active_task_id() == task_id_; Do we need to maintain two separate notions of Active? When are they ever in conflict? Should we instead just DCHECK that one is in sync with the other? (if they are indeed changed together synchronously). https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:124: bool ArcAppWindow::IsFullscreen() const { Shouldn't this tie into your fullscreen setting? I guess it doesn't matter if it's never called, but with all the NOTREACHED()s here, I wonder why we need this ui::BaseWindow in the first place (and not just use views::Widgets and/or aura::Windows). https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:201: void ArcAppWindow::ResetIcon() { Is this actually useful? Can we just use the current icon until it changes? (see other comments). It seems like the worst case scenario is that an app window had a valid icon, and somehow changed it, and that decode fails, so we continue showing the old icon... it seems worth simplifying the code if we just don't handle odd corner cases like that. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:209: image_skia_ = gfx::ImageSkia(new CustomIconSource(decoded_image), It seems like this calls shouldn't store a separate icon, but instead set the aura::client::kAppIconKey or kWindowIconKey property on the underlying aura::Window. Generally, it's better to re-use existing infrastructure and patterns instead of adding new ones that work differently to accomplish a similar goal. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:216: ResetIcon(); Why should the controller be notified when the icon decoding fails? Can it just continue using the existing (perhaps empty) icon and not need to be notified? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:26: class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest { It's too bad that we can't use and extend an existing ui::BaseWindow subclass, like NativeAppWindow (for extensions). Or simply let the [Item]Controllers deal with views::Widgets and aura::Windows. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:28: enum class FullScreenMode { Remove this and use a bool set to false by default, or use an existing enum, like ash::wm::WindowStateType and the existing ui::BaseWindow::IsFullscreen(). Avoid adding duplicate types/functions that someone will need to combine later. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:41: static void DisableSafeIconDecodingForTesting(); Add comments for all non-obvious (non-accessor/setter) functions. (ie. a reader should understand why we'd disable safe icon decoding for testing) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:45: void ResetController(); Remove this, let users call SetController(nullptr) as needed. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:50: void SetDescription(const std::string& title, Should this be two separate functions? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:51: const std::vector<uint8_t>& unsafe_icon_data_png); nit: "unsafe" needs a comment. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:65: ArcAppWindowLauncherItemController* controller() { return controller_; } nit: const? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:67: const gfx::ImageSkia& image_skia() { return image_skia_; } nit: const? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:93: // Resets custom icon if it was previously set. If current window is an active nit: maybe just " // Resets the icon and updates |controller_|'s active icon as needed." (if you need to keep this function, hopefully not) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:101: const int task_id_; Please comment on all members. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:105: gfx::ImageSkia image_skia_; nit: Rename this and the accessor to convey the purpose of this image_skia (ie. icon) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:68: class ArcAppWindowLauncherController::AppWindowInfo { I wonder if we could fix ArcAppWindow lifetimes so this class isn't needed. (ie. create instances earlier to store some pre-initialized state) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:87: app_window_->SetDescription(title_, icon_data_png_); Should the ArcAppWindow constructor just take a title and icon, and restore this function to a simple setter? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:455: if (!info) nit: invert conditional and nest the function call https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:76: void OnTaskSetDescription(int32_t task_id, nit: order to match interface decl https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:96: if (image_set_by_controller()) { Can we avoid using image_set_by_controller and just set the aura::Window icon properties? It would be nice to use generalized behavior (in AppWindowLauncher[Item]Controller), instead of developing arc-specific code to do something that all shelf items should do. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h:38: void OnWindowChanged(ArcAppWindow* arc_app_window); nit: comment. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h:41: // Updates controller icon from the active window. If custom icon of the nit: maybe simplify the comment? eg. "Update the shelf item's icon for the active window". Perhaps this would change if we use aura::Window icon properties. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2297: ArcAppWindow::DisableSafeIconDecodingForTesting(); Would it be reasonable for ArcAppWindow to check the setting on ArcAppIcon, and just have one of these flags, instead of needing to set both? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2302: std::string png_data; nit: add more comments to explain what these first groupings of statements do? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2333: // Set custom icon on active item. Icon should be change to custom. nit: 'be changed' or just 'should change' https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2341: // Test that setting of invalid icon resets the custom icon. nit: 'setting an' https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2348: // Check window removing with active custom icon. nit: move comment down to where you're actually removing a window and change this comment to what's actually happening in the next two lines. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2351: // Reseting custom icon of inactive window doesn't reset shefl icon. nit: "shelf"
Thanks for comments. PTAL updated version https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1066: void ArcAppListPrefs::OnTaskSetDescription( On 2017/05/18 18:26:47, msw wrote: > ditto nit: order to match interface decl Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:133: virtual void OnTaskSetDescription( On 2017/05/18 18:26:47, msw wrote: > nit: consider OnTaskDescriptionChanged, OnTaskDescriptionUpdated or similar. (or > Changing/Updating if it has yet to actually change) Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:299: void OnTaskSetDescription(int32_t task_id, On 2017/05/18 18:26:47, msw wrote: > nit: order to match interface decl. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:82: // Returns last active window in the controller or top window. On 2017/05/18 18:26:47, msw wrote: > nit: s/top/first/? (to avoid confusion with MRU, z-order, or placement) > aside (for another cl): should something actually track MRU windows (maybe using > MruWindowTracker and checking against |windows_|) to avoid just returning 'any' > window if the |last_active_window_| is removed? Seems like we could maybe even > get rid of |last_active_window_| in that case. Agree that last_active_window_ looks good target to refactor a bit. Comment updated. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:83: ui::BaseWindow* GetLastActiveWindow(); On 2017/05/18 18:26:48, msw wrote: > nit: make this private if it's not used outside this class. It is used from ArcAppWindowLauncherItemController. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:21: // Set to true for unit tests to avoid IPC icon decoding. On 2017/05/18 18:26:48, msw wrote: > nit: explain why that's useful. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:22: bool disable_safe_icon_decoding = false; On 2017/05/18 18:26:48, msw wrote: > nit: append _for_testing to the identifier itself Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:89: if (title.length()) On 2017/05/18 18:26:48, msw wrote: > nit !empty? Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:90: GetNativeWindow()->SetTitle(base::string16(base::UTF8ToUTF16(title))); On 2017/05/18 18:26:48, msw wrote: > nit: base::string16 is probably not needed (adds a copy) Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:93: ResetIcon(); On 2017/05/18 18:26:48, msw wrote: > Can we just continue using the current icon until a new one is successfully > decoded? It would be nice to avoid unnecessary work/code and avoid blank icons > flashing during decode, etc. In this case we pass empty data which means custom icon is reset. ResetIcon just discard (if it was previously set) custom icon. No blank icon should appear: * If we set custom icon then we try to decode it first and on decode completed we update actual icon. During the decoding default or previous custom icon is used. * If we reset custom icon we immediately discards inline previous custom icon and default icon is used. Also no blank icon. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:111: return widget_->IsActive() && owner_->active_task_id() == task_id_; On 2017/05/18 18:26:48, msw wrote: > Do we need to maintain two separate notions of Active? When are they ever in > conflict? Should we instead just DCHECK that one is in sync with the other? (if > they are indeed changed together synchronously). Activating ARC app is not straightforward. It may be activated from Android side or by clicking on corresponded Chrome window. It took a time to stabilize focus and active state of ARC windows. Anyway this code is legacy and I don't modify it here. If we want to reconsider how it work let do it in separate task. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:124: bool ArcAppWindow::IsFullscreen() const { On 2017/05/18 18:26:48, msw wrote: > Shouldn't this tie into your fullscreen setting? I guess it doesn't matter if > it's never called, but with all the NOTREACHED()s here, I wonder why we need > this ui::BaseWindow in the first place (and not just use views::Widgets and/or > aura::Windows). Similar to previous comment. I would prefer not to experiment with legacy code in this CL. Let do as separate task. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:201: void ArcAppWindow::ResetIcon() { On 2017/05/18 18:26:48, msw wrote: > Is this actually useful? Can we just use the current icon until it changes? (see > other comments). It seems like the worst case scenario is that an app window had > a valid icon, and somehow changed it, and that decode fails, so we continue > showing the old icon... it seems worth simplifying the code if we just don't > handle odd corner cases like that. I explained in previous comment. App may pass null as task bitmap. In this case icon_png_data is empty and we reset the icon to default activity icon. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:209: image_skia_ = gfx::ImageSkia(new CustomIconSource(decoded_image), On 2017/05/18 18:26:48, msw wrote: > It seems like this calls shouldn't store a separate icon, but instead set the > aura::client::kAppIconKey or kWindowIconKey property on the underlying > aura::Window. Generally, it's better to re-use existing infrastructure and > patterns instead of adding new ones that work differently to accomplish a > similar goal. Hmm. I did similar as extension app window works. https://cs.chromium.org does not work currently. You may take a look: chromium/src/extensions/browser/app_window/app_window.h const gfx::Image& app_icon() const { return app_icon_; } https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:216: ResetIcon(); On 2017/05/18 18:26:48, msw wrote: > Why should the controller be notified when the icon decoding fails? Can it just > continue using the existing (perhaps empty) icon and not need to be notified? We probably may continue using old icon. And this should be exceptional case, once we were able to compress icon to png on ARC side. Removed this one https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:28: enum class FullScreenMode { On 2017/05/18 18:26:50, msw wrote: > Remove this and use a bool set to false by default, or use an existing enum, > like ash::wm::WindowStateType and the existing ui::BaseWindow::IsFullscreen(). > Avoid adding duplicate types/functions that someone will need to combine later. This is legacy code and I don't touch this functionality in this CL. Let do it separately. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:41: static void DisableSafeIconDecodingForTesting(); On 2017/05/18 18:26:56, msw wrote: > Add comments for all non-obvious (non-accessor/setter) functions. (ie. a reader > should understand why we'd disable safe icon decoding for testing) Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:45: void ResetController(); On 2017/05/18 18:26:53, msw wrote: > Remove this, let users call SetController(nullptr) as needed. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:50: void SetDescription(const std::string& title, On 2017/05/18 18:26:49, msw wrote: > Should this be two separate functions? They are set together. Imho we can handle it together as well. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:51: const std::vector<uint8_t>& unsafe_icon_data_png); On 2017/05/18 18:26:58, msw wrote: > nit: "unsafe" needs a comment. Done https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:65: ArcAppWindowLauncherItemController* controller() { return controller_; } On 2017/05/18 18:26:50, msw wrote: > nit: const? Done. However in one of my previous CL reviewer told me if we return non-constant member (that time it was also pointer) we should not set const. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:67: const gfx::ImageSkia& image_skia() { return image_skia_; } On 2017/05/18 18:26:59, msw wrote: > nit: const? Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:93: // Resets custom icon if it was previously set. If current window is an active On 2017/05/18 18:26:57, msw wrote: > nit: maybe just " // Resets the icon and updates |controller_|'s active icon as > needed." (if you need to keep this function, hopefully not) I described in other comments that user may want to reset previously set custom icon and this is valid case (icon in Android TaskDescription is optional value). Comment updated. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:101: const int task_id_; On 2017/05/18 18:26:52, msw wrote: > Please comment on all members. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:68: class ArcAppWindowLauncherController::AppWindowInfo { On 2017/05/18 18:26:59, msw wrote: > I wonder if we could fix ArcAppWindow lifetimes so this class isn't needed. > (ie. create instances earlier to store some pre-initialized state) That is needed, in ARC AppWindow is optional and may be re-created several time. Most easy to achieve this by switching some app into true full-screen mode. In this case other windows are closed but not the task. And info contains information required to restore the window state when it is needed. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:87: app_window_->SetDescription(title_, icon_data_png_); On 2017/05/18 18:26:59, msw wrote: > Should the ArcAppWindow constructor just take a title and icon, and restore this > function to a simple setter? Restored to setter. However it seems adding this to CTOR is redundant. If we pass it to CTOR it would immediately call SetDescription internally. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:455: if (!info) On 2017/05/18 18:26:59, msw wrote: > nit: invert conditional and nest the function call Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:76: void OnTaskSetDescription(int32_t task_id, On 2017/05/18 18:26:59, msw wrote: > nit: order to match interface decl Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:96: if (image_set_by_controller()) { On 2017/05/18 18:27:00, msw wrote: > Can we avoid using image_set_by_controller and just set the aura::Window icon > properties? It would be nice to use generalized behavior (in > AppWindowLauncher[Item]Controller), instead of developing arc-specific code to > do something that all shelf items should do. I did similar as extension app window controller does. If we want to change let change it together. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h:38: void OnWindowChanged(ArcAppWindow* arc_app_window); On 2017/05/18 18:27:00, msw wrote: > nit: comment. Discarded in new code. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h:41: // Updates controller icon from the active window. If custom icon of the On 2017/05/18 18:27:00, msw wrote: > nit: maybe simplify the comment? eg. "Update the shelf item's icon for the > active window". Perhaps this would change if we use aura::Window icon > properties. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2297: ArcAppWindow::DisableSafeIconDecodingForTesting(); On 2017/05/18 18:27:00, msw wrote: > Would it be reasonable for ArcAppWindow to check the setting on ArcAppIcon, and > just have one of these flags, instead of needing to set both? Added getter to ArcAppIcon https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2302: std::string png_data; On 2017/05/18 18:27:00, msw wrote: > nit: add more comments to explain what these first groupings of statements do? Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2333: // Set custom icon on active item. Icon should be change to custom. On 2017/05/18 18:27:00, msw wrote: > nit: 'be changed' or just 'should change' Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2341: // Test that setting of invalid icon resets the custom icon. On 2017/05/18 18:27:00, msw wrote: > nit: 'setting an' Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2348: // Check window removing with active custom icon. On 2017/05/18 18:27:00, msw wrote: > nit: move comment down to where you're actually removing a window and change > this comment to what's actually happening in the next two lines. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2351: // Reseting custom icon of inactive window doesn't reset shefl icon. On 2017/05/18 18:27:00, msw wrote: > nit: "shelf" Done. https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:127: return windows_.empty() ? nullptr : windows_.front(); On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > nit: > > if (windows_.empty()) > return nullptr > return windows_.front(); Done. https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:89: if (!arc_app_window || arc_app_window->image_skia().isNull()) { On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > nit: use the guard clause pattern: > > if (!arc_app_window || arc_app_window->image_skia().isNull()) { > if (!image_set_by_controller()) > return; > ... > return; > } > > owner_->... Done. https://codereview.chromium.org/2894443002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2894443002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:131: // Sends task description. On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > nit: Sends task label and icon. Done. https://codereview.chromium.org/2894443002/diff/80001/components/arc/test/fak... File components/arc/test/fake_app_instance.h (right): https://codereview.chromium.org/2894443002/diff/80001/components/arc/test/fak... components/arc/test/fake_app_instance.h:126: const std::string& icon_png_data); On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > nit: icon_png_data_as_string (for consistency with the other functions). Done.
https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:26: class CustomIconSource : public gfx::ImageSkiaSource { On 2017/05/18 17:29:34, khmel wrote: > On 2017/05/18 16:22:49, xiyuan wrote: > > Can we get rid of this class and do something like the following ? > > > > image_skia_ = gfx::ImageSkiaOperations::CreateResizedImage( > > gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f), > > gfx::Size(extension_misc::EXTENSION_ICON_SMALL, > > extension_misc::EXTENSION_ICON_SMALL)); > > > > Think that should give us the same resized icon. > > Yes, if required scale is missing then 1.0 is taken and scaled. Done Nice! https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:93: ResetIcon(); On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:26:48, msw wrote: > > Can we just continue using the current icon until a new one is successfully > > decoded? It would be nice to avoid unnecessary work/code and avoid blank icons > > flashing during decode, etc. > > In this case we pass empty data which means custom icon is reset. ResetIcon just > discard (if it was previously set) custom icon. No blank icon should appear: > * If we set custom icon then we try to decode it first and on decode completed > we update actual icon. During the decoding default or previous custom icon is > used. > * If we reset custom icon we immediately discards inline previous custom icon > and default icon is used. Also no blank icon. So you're trying to use the app/activity icon instead of the window icon in this case? That might make sense, but it would be much clearer if both this class and the extension version dealt directly with the app and window icon properties, and then the AppWindowLauncher[Item]Controller handled window properties changing in a common way. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:111: return widget_->IsActive() && owner_->active_task_id() == task_id_; On 2017/05/18 20:27:50, khmel wrote: > On 2017/05/18 18:26:48, msw wrote: > > Do we need to maintain two separate notions of Active? When are they ever in > > conflict? Should we instead just DCHECK that one is in sync with the other? > (if > > they are indeed changed together synchronously). > > Activating ARC app is not straightforward. It may be activated from Android side > or by clicking on corresponded Chrome window. It took a time to stabilize focus > and active state of ARC windows. Anyway this code is legacy and I don't modify > it here. If we want to reconsider how it work let do it in separate task. It would be nice to clean this up later, this is technical debt. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:124: bool ArcAppWindow::IsFullscreen() const { On 2017/05/18 20:27:50, khmel wrote: > On 2017/05/18 18:26:48, msw wrote: > > Shouldn't this tie into your fullscreen setting? I guess it doesn't matter if > > it's never called, but with all the NOTREACHED()s here, I wonder why we need > > this ui::BaseWindow in the first place (and not just use views::Widgets and/or > > aura::Windows). > > Similar to previous comment. I would prefer not to experiment with legacy code > in this CL. Let do as separate task. Acknowledged. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:209: image_skia_ = gfx::ImageSkia(new CustomIconSource(decoded_image), On 2017/05/18 20:27:50, khmel wrote: > On 2017/05/18 18:26:48, msw wrote: > > It seems like this calls shouldn't store a separate icon, but instead set the > > aura::client::kAppIconKey or kWindowIconKey property on the underlying > > aura::Window. Generally, it's better to re-use existing infrastructure and > > patterns instead of adding new ones that work differently to accomplish a > > similar goal. > > Hmm. I did similar as extension app window works. https://cs.chromium.org does > not work currently. You may take a look: > chromium/src/extensions/browser/app_window/app_window.h > const gfx::Image& app_icon() const { return app_icon_; } We should consolidate common behavior on the reusable AppWindowLauncher[Item]Controller base classes, instead of duplicating one-off behavior between the extension/arc subclasses. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:28: enum class FullScreenMode { On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:26:50, msw wrote: > > Remove this and use a bool set to false by default, or use an existing enum, > > like ash::wm::WindowStateType and the existing ui::BaseWindow::IsFullscreen(). > > Avoid adding duplicate types/functions that someone will need to combine > later. > > This is legacy code and I don't touch this functionality in this CL. Let do it > separately. Add a TODO, this is technical debt. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:65: ArcAppWindowLauncherItemController* controller() { return controller_; } On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:26:50, msw wrote: > > nit: const? > > Done. However in one of my previous CL reviewer told me if we return > non-constant member (that time it was also pointer) we should not set const. Ah, good point; feel free to keep this non-const. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:68: class ArcAppWindowLauncherController::AppWindowInfo { On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:26:59, msw wrote: > > I wonder if we could fix ArcAppWindow lifetimes so this class isn't needed. > > (ie. create instances earlier to store some pre-initialized state) > > That is needed, in ARC AppWindow is optional and may be re-created several time. > Most easy to achieve this by switching some app into true full-screen mode. In > this case other windows are closed but not the task. And info contains > information required to restore the window state when it is needed. It's outside the scope of this CL, but couldn't we just separate ArcAppWindow lifetime from the underlying aura::Window or views::Widget lifetime? That way, we wouldn't need this info class, and the ArcAppWindow class would just handle its windows being created/initialized/destroyed as needed? https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:87: app_window_->SetDescription(title_, icon_data_png_); On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:26:59, msw wrote: > > Should the ArcAppWindow constructor just take a title and icon, and restore > this > > function to a simple setter? > > Restored to setter. However it seems adding this to CTOR is redundant. If we > pass it to CTOR it would immediately call SetDescription internally. Your updated change here seems fine. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:96: if (image_set_by_controller()) { On 2017/05/18 20:27:51, khmel wrote: > On 2017/05/18 18:27:00, msw wrote: > > Can we avoid using image_set_by_controller and just set the aura::Window icon > > properties? It would be nice to use generalized behavior (in > > AppWindowLauncher[Item]Controller), instead of developing arc-specific code to > > do something that all shelf items should do. > > I did similar as extension app window controller does. If we want to change let > change it together. Yes, both should be simplified, rather than copying one-off code. https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:127: return windows_.empty() ? nullptr : windows_.front(); On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > nit: > > if (windows_.empty()) > return nullptr > return windows_.front(); fwiw, I prefer ternary statements, but style preferences aren't too important. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.h (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.h:61: optional nit: remove blank line https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:63: // Called when launcher item may need to be updated, for example label or optional nit: s/for example/eg./ for one-liner. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:65: virtual void UpdateLauncherItem() {} This should be replaced by OnWindowPropertyChanged (above) handling aura::Window icon property changes, ideally in a similar manner for both Arc and Extensions, handled in this base class. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2353: // Reseting custom icon of inactive window doesn't reset shelf icon. optional nit: combine comment with the line above?
https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:26: class CustomIconSource : public gfx::ImageSkiaSource { On 2017/05/18 22:03:38, msw wrote: > On 2017/05/18 17:29:34, khmel wrote: > > On 2017/05/18 16:22:49, xiyuan wrote: > > > Can we get rid of this class and do something like the following ? > > > > > > image_skia_ = gfx::ImageSkiaOperations::CreateResizedImage( > > > gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f), > > > gfx::Size(extension_misc::EXTENSION_ICON_SMALL, > > > extension_misc::EXTENSION_ICON_SMALL)); > > > > > > Think that should give us the same resized icon. > > > > Yes, if required scale is missing then 1.0 is taken and scaled. Done > > Nice! Acknowledged. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:93: ResetIcon(); On 2017/05/18 22:03:38, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:26:48, msw wrote: > > > Can we just continue using the current icon until a new one is successfully > > > decoded? It would be nice to avoid unnecessary work/code and avoid blank > icons > > > flashing during decode, etc. > > > > In this case we pass empty data which means custom icon is reset. ResetIcon > just > > discard (if it was previously set) custom icon. No blank icon should appear: > > * If we set custom icon then we try to decode it first and on decode > completed > > we update actual icon. During the decoding default or previous custom icon is > > used. > > * If we reset custom icon we immediately discards inline previous custom icon > > and default icon is used. Also no blank icon. > > So you're trying to use the app/activity icon instead of the window icon in this > case? That might make sense, but it would be much clearer if both this class and > the extension version dealt directly with the app and window icon properties, > and then the AppWindowLauncher[Item]Controller handled window properties > changing in a common way. This makes sense but sounds as refactoring, quite big as I predict. I think let do it as separate CL. In meantime it is good to have everything in one style. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:111: return widget_->IsActive() && owner_->active_task_id() == task_id_; On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:50, khmel wrote: > > On 2017/05/18 18:26:48, msw wrote: > > > Do we need to maintain two separate notions of Active? When are they ever in > > > conflict? Should we instead just DCHECK that one is in sync with the other? > > (if > > > they are indeed changed together synchronously). > > > > Activating ARC app is not straightforward. It may be activated from Android > side > > or by clicking on corresponded Chrome window. It took a time to stabilize > focus > > and active state of ARC windows. Anyway this code is legacy and I don't modify > > it here. If we want to reconsider how it work let do it in separate task. > > It would be nice to clean this up later, this is technical debt. Acknowledged. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:209: image_skia_ = gfx::ImageSkia(new CustomIconSource(decoded_image), On 2017/05/18 22:03:38, msw wrote: > On 2017/05/18 20:27:50, khmel wrote: > > On 2017/05/18 18:26:48, msw wrote: > > > It seems like this calls shouldn't store a separate icon, but instead set > the > > > aura::client::kAppIconKey or kWindowIconKey property on the underlying > > > aura::Window. Generally, it's better to re-use existing infrastructure and > > > patterns instead of adding new ones that work differently to accomplish a > > > similar goal. > > > > Hmm. I did similar as extension app window works. https://cs.chromium.org does > > not work currently. You may take a look: > > chromium/src/extensions/browser/app_window/app_window.h > > const gfx::Image& app_icon() const { return app_icon_; } > > We should consolidate common behavior on the reusable > AppWindowLauncher[Item]Controller base classes, instead of duplicating one-off > behavior between the extension/arc subclasses. As above, I suggest to clean up this as separate CL. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.h (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:26: class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest { On 2017/05/18 18:26:51, msw wrote: > It's too bad that we can't use and extend an existing ui::BaseWindow subclass, > like NativeAppWindow (for extensions). Or simply let the [Item]Controllers deal > with views::Widgets and aura::Windows. Probably, but that is outside of context of this CL. IIRC AppWindow for extension has long history and was already sub-classed from ui::BaseWindow. Arc launcher item just inherited this concept. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:28: enum class FullScreenMode { On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:26:50, msw wrote: > > > Remove this and use a bool set to false by default, or use an existing enum, > > > like ash::wm::WindowStateType and the existing > ui::BaseWindow::IsFullscreen(). > > > Avoid adding duplicate types/functions that someone will need to combine > > later. > > > > This is legacy code and I don't touch this functionality in this CL. Let do it > > separately. > > Add a TODO, this is technical debt. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:65: ArcAppWindowLauncherItemController* controller() { return controller_; } On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:26:50, msw wrote: > > > nit: const? > > > > Done. However in one of my previous CL reviewer told me if we return > > non-constant member (that time it was also pointer) we should not set const. > > > Ah, good point; feel free to keep this non-const. Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.h:105: gfx::ImageSkia image_skia_; On 2017/05/18 18:26:55, msw wrote: > nit: Rename this and the accessor to convey the purpose of this image_skia (ie. > icon) Done. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:68: class ArcAppWindowLauncherController::AppWindowInfo { On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:26:59, msw wrote: > > > I wonder if we could fix ArcAppWindow lifetimes so this class isn't needed. > > > (ie. create instances earlier to store some pre-initialized state) > > > > That is needed, in ARC AppWindow is optional and may be re-created several > time. > > Most easy to achieve this by switching some app into true full-screen mode. In > > this case other windows are closed but not the task. And info contains > > information required to restore the window state when it is needed. > > It's outside the scope of this CL, but couldn't we just separate ArcAppWindow > lifetime from the underlying aura::Window or views::Widget lifetime? That way, > we wouldn't need this info class, and the ArcAppWindow class would just handle > its windows being created/initialized/destroyed as needed? +1 to try to do this. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:87: app_window_->SetDescription(title_, icon_data_png_); On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:26:59, msw wrote: > > > Should the ArcAppWindow constructor just take a title and icon, and restore > > this > > > function to a simple setter? > > > > Restored to setter. However it seems adding this to CTOR is redundant. If we > > pass it to CTOR it would immediately call SetDescription internally. > > Your updated change here seems fine. Acknowledged. https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:96: if (image_set_by_controller()) { On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 20:27:51, khmel wrote: > > On 2017/05/18 18:27:00, msw wrote: > > > Can we avoid using image_set_by_controller and just set the aura::Window > icon > > > properties? It would be nice to use generalized behavior (in > > > AppWindowLauncher[Item]Controller), instead of developing arc-specific code > to > > > do something that all shelf items should do. > > > > I did similar as extension app window controller does. If we want to change > let > > change it together. > > Yes, both should be simplified, rather than copying one-off code. Add crbug and TODO for refactoring both. https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc (right): https://codereview.chromium.org/2894443002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:127: return windows_.empty() ? nullptr : windows_.front(); On 2017/05/18 22:03:39, msw wrote: > On 2017/05/18 18:09:40, Luis Héctor Chávez wrote: > > nit: > > > > if (windows_.empty()) > > return nullptr > > return windows_.front(); > > fwiw, I prefer ternary statements, but style preferences aren't too important. Acknowledged. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.h (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.h:61: On 2017/05/18 22:03:39, msw wrote: > optional nit: remove blank line Done. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:63: // Called when launcher item may need to be updated, for example label or On 2017/05/18 22:03:39, msw wrote: > optional nit: s/for example/eg./ for one-liner. Done. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:65: virtual void UpdateLauncherItem() {} On 2017/05/18 22:03:39, msw wrote: > This should be replaced by OnWindowPropertyChanged (above) handling aura::Window > icon property changes, ideally in a similar manner for both Arc and Extensions, > handled in this base class. I added TODO. This sounds as good point. I can do it in next CL. https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc (right): https://codereview.chromium.org/2894443002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:2353: // Reseting custom icon of inactive window doesn't reset shelf icon. On 2017/05/18 22:03:39, msw wrote: > optional nit: combine comment with the line above? Done.
lgtm as temporary code before addressing crbug.com/724292 (this CL copies technical debt, and should be refactored) https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_window.cc (right): https://codereview.chromium.org/2894443002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_window.cc:93: ResetIcon(); On 2017/05/18 22:40:02, khmel wrote: > On 2017/05/18 22:03:38, msw wrote: > > On 2017/05/18 20:27:51, khmel wrote: > > > On 2017/05/18 18:26:48, msw wrote: > > > > Can we just continue using the current icon until a new one is > successfully > > > > decoded? It would be nice to avoid unnecessary work/code and avoid blank > > icons > > > > flashing during decode, etc. > > > > > > In this case we pass empty data which means custom icon is reset. ResetIcon > > just > > > discard (if it was previously set) custom icon. No blank icon should appear: > > > * If we set custom icon then we try to decode it first and on decode > > completed > > > we update actual icon. During the decoding default or previous custom icon > is > > > used. > > > * If we reset custom icon we immediately discards inline previous custom > icon > > > and default icon is used. Also no blank icon. > > > > So you're trying to use the app/activity icon instead of the window icon in > this > > case? That might make sense, but it would be much clearer if both this class > and > > the extension version dealt directly with the app and window icon properties, > > and then the AppWindowLauncher[Item]Controller handled window properties > > changing in a common way. > > This makes sense but sounds as refactoring, quite big as I predict. I think let > do it as separate CL. In meantime it is good to have everything in one style. I look forward to seeing the cleanup change for http://crbug.com/724292. It's *bad* to duplicate old complex code and increase technical debt.
Hi Nasko, Gentle ping (components/arc/common/app.mojom) Thanks!
https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:76: title_ = title; This data comes from untrusted process, so some verification should be performed. Is there any limit that applies to titles? Is it expected to be UTF-8 or some other encoding? https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; I don't know if there is much to be verified here, but if there are any known constraints on the data, it will be good to check them here.
https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:76: title_ = title; On 2017/05/23 05:02:31, nasko wrote: > This data comes from untrusted process, so some verification should be > performed. Is there any limit that applies to titles? Is it expected to be UTF-8 > or some other encoding? Yes, it is expected that this is UTF_8. IIUC UTF8ToUTF16 is safe. https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; On 2017/05/23 05:02:31, nasko wrote: > I don't know if there is much to be verified here, but if there are any known > constraints on the data, it will be good to check them here. I can suggest to limit this by size. Normally this is around 6-12kb. I think 64 kb limit could be good for this. Besides, png data is decoded in sandboxed process so it should be safe.
https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; On 2017/05/23 at 15:33:28, khmel wrote: > On 2017/05/23 05:02:31, nasko wrote: > > I don't know if there is much to be verified here, but if there are any known > > constraints on the data, it will be good to check them here. > > I can suggest to limit this by size. Normally this is around 6-12kb. I think 64 kb limit could be good for this. Besides, png data is decoded in sandboxed process so it should be safe. Oh, didn't know we used a sandboxed process for this. What process is that? Do we have a dedicated image decode process in chrome that I wasn't aware of?
https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; On 2017/05/23 16:46:17, reveman wrote: > On 2017/05/23 at 15:33:28, khmel wrote: > > On 2017/05/23 05:02:31, nasko wrote: > > > I don't know if there is much to be verified here, but if there are any > known > > > constraints on the data, it will be good to check them here. > > > > I can suggest to limit this by size. Normally this is around 6-12kb. I think > 64 kb limit could be good for this. Besides, png data is decoded in sandboxed > process so it should be safe. > > Oh, didn't know we used a sandboxed process for this. What process is that? Do > we have a dedicated image decode process in chrome that I wasn't aware of? I know only high level details: https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&l=24 This is way we work with ARC icons. We already used this approach for decoding default app icon, shortcuts icon (probably more).
I still think we can do better validation on the string input and thanks for checking the icon size! https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:76: title_ = title; On 2017/05/23 15:33:28, khmel wrote: > On 2017/05/23 05:02:31, nasko wrote: > > This data comes from untrusted process, so some verification should be > > performed. Is there any limit that applies to titles? Is it expected to be > UTF-8 > > or some other encoding? > > Yes, it is expected that this is UTF_8. IIUC UTF8ToUTF16 is safe. It is not just a matter whether the conversion functions are safe or not, the data comes from untrusted source, so we need to ensure we validate it. Why not pass it through base::IsStringUTF8? https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; On 2017/05/23 16:52:49, khmel wrote: > On 2017/05/23 16:46:17, reveman wrote: > > On 2017/05/23 at 15:33:28, khmel wrote: > > > On 2017/05/23 05:02:31, nasko wrote: > > > > I don't know if there is much to be verified here, but if there are any > > known > > > > constraints on the data, it will be good to check them here. > > > > > > I can suggest to limit this by size. Normally this is around 6-12kb. I think > > 64 kb limit could be good for this. Besides, png data is decoded in sandboxed > > process so it should be safe. > > > > Oh, didn't know we used a sandboxed process for this. What process is that? Do > > we have a dedicated image decode process in chrome that I wasn't aware of? > > I know only high level details: > https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&l=24 > This is way we work with ARC icons. We already used this approach for decoding > default app icon, shortcuts icon (probably more). While decoding in sandboxed process is awesome for security, we still need to validate data coming from that process. If the data is specially crafted to exploit a bug and get arbitrary code execution, then the sandboxed process can easily send us completely bogus data or one specially crafted to exploit other memory bugs in the more privileged process. Thanks for adding the size check!
https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:76: title_ = title; On 2017/05/23 21:52:29, nasko wrote: > On 2017/05/23 15:33:28, khmel wrote: > > On 2017/05/23 05:02:31, nasko wrote: > > > This data comes from untrusted process, so some verification should be > > > performed. Is there any limit that applies to titles? Is it expected to be > > UTF-8 > > > or some other encoding? > > > > Yes, it is expected that this is UTF_8. IIUC UTF8ToUTF16 is safe. > > It is not just a matter whether the conversion functions are safe or not, the > data comes from untrusted source, so we need to ensure we validate it. Why not > pass it through base::IsStringUTF8? I tried to find similar but could not (well, name itself is simple :)). Thank you for reference! Done. https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; On 2017/05/23 21:52:29, nasko wrote: > On 2017/05/23 16:52:49, khmel wrote: > > On 2017/05/23 16:46:17, reveman wrote: > > > On 2017/05/23 at 15:33:28, khmel wrote: > > > > On 2017/05/23 05:02:31, nasko wrote: > > > > > I don't know if there is much to be verified here, but if there are any > > > known > > > > > constraints on the data, it will be good to check them here. > > > > > > > > I can suggest to limit this by size. Normally this is around 6-12kb. I > think > > > 64 kb limit could be good for this. Besides, png data is decoded in > sandboxed > > > process so it should be safe. > > > > > > Oh, didn't know we used a sandboxed process for this. What process is that? > Do > > > we have a dedicated image decode process in chrome that I wasn't aware of? > > > > I know only high level details: > > > https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&l=24 > > This is way we work with ARC icons. We already used this approach for decoding > > default app icon, shortcuts icon (probably more). > > While decoding in sandboxed process is awesome for security, we still need to > validate data coming from that process. If the data is specially crafted to > exploit a bug and get arbitrary code execution, then the sandboxed process can > easily send us completely bogus data or one specially crafted to exploit other > memory bugs in the more privileged process. > > Thanks for adding the size check! Acknowledged. Thanks for sharing your knowledge!
On 2017/05/23 at 16:52:50, khmel wrote: > https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): > > https://codereview.chromium.org/2894443002/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:79: icon_data_png_ = icon_data_png; > On 2017/05/23 16:46:17, reveman wrote: > > On 2017/05/23 at 15:33:28, khmel wrote: > > > On 2017/05/23 05:02:31, nasko wrote: > > > > I don't know if there is much to be verified here, but if there are any > > known > > > > constraints on the data, it will be good to check them here. > > > > > > I can suggest to limit this by size. Normally this is around 6-12kb. I think > > 64 kb limit could be good for this. Besides, png data is decoded in sandboxed > > process so it should be safe. > > > > Oh, didn't know we used a sandboxed process for this. What process is that? Do > > we have a dedicated image decode process in chrome that I wasn't aware of? > > I know only high level details: > https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&l=24 > This is way we work with ARC icons. We already used this approach for decoding default app icon, shortcuts icon (probably more). Ok, good. I'm a bit surprised that we're not just doing this decode on the Android side instead so we can avoid the complexity and overhead of having to spin up a sandboxed decode process. Anyhow, if it's done in sandboxed utility process then that seems secure at least.
mojo LGTM
Thank you for review! Will try to land...
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, xiyuan@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2894443002/#ps160001 (title: "add label check")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, lhchavez@chromium.org, nasko@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2894443002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495724350975290,
"parent_rev": "ca8346a08cd5b89f5a957a7c583572770c9a4e43", "commit_rev":
"f4821feb1dfd4ad2b98ffc4be0c7deae02fa7e82"}
Message was sent while issue was closed.
Description was changed from ========== arc: Set custom icon in shelf for ARC apps. This handles setTaskDescription requests which provides optional icon for the task. This CL receives icon from ARC and associates it with ARC app window. ARC app window contoller receives updates and update controller item accordingly. If no custom icon set then default app icon is used. Preview: https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qM3FTYWVJdl9FMDA/vie... TEST=Manually + unit_test BUG=723742 ========== to ========== arc: Set custom icon in shelf for ARC apps. This handles setTaskDescription requests which provides optional icon for the task. This CL receives icon from ARC and associates it with ARC app window. ARC app window contoller receives updates and update controller item accordingly. If no custom icon set then default app icon is used. Preview: https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qM3FTYWVJdl9FMDA/vie... TEST=Manually + unit_test BUG=723742 Review-Url: https://codereview.chromium.org/2894443002 Cr-Commit-Position: refs/heads/master@{#474663} Committed: https://chromium.googlesource.com/chromium/src/+/f4821feb1dfd4ad2b98ffc4be0c7... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/f4821feb1dfd4ad2b98ffc4be0c7... |
