Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209053004:
Enhance chrome.app.window API with icon property (Closed)
|
Created:
4 years, 4 months ago by Andra Paraschiv Modified:
4 years, 4 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnhance chrome.app.window API with icon property
Enhance chrome.app.window API with the possibility of creating a window that
has its own icon in the shelf.
Added a new property, icon, to CreateWindowOptions.
Based on https://codereview.chromium.org/1914993002
Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>;
BUG=610299
TEST=interactive_ui_tests
Implement a simple extension that creates windows with icon property set to an
image url. Observe that the newly created windows have their own icons in the
shelf.
Committed: https://crrev.com/ebbb79d24ceb6625788dc9b121b880700b6340d6
Cr-Commit-Position: refs/heads/master@{#412185}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Review v2 + Badge Icon #
Total comments: 16
Patch Set 3 : Review v3 #Patch Set 4 : Fix AppWindow Comment #
Total comments: 2
Patch Set 5 : Review v4 #
Total comments: 4
Patch Set 6 : Review v5 #
Messages
Total messages: 34 (10 generated)
Description was changed from ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. ========== to ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. ==========
andra.paraschiv@intel.com changed reviewers: + reillyg@chromium.org, skuhne@chromium.org, stevenjb@chromium.org
Uploaded the basic functionality for the showInShelf=true icon property. Could you please take a look? https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... extensions/browser/app_window/app_window.cc:313: web_contents()->DownloadImage( This function can fetch the icon in an async way and there could be some delays in loading the image. https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... extensions/browser/app_window/app_window.cc:626: window_icon_ = image; Currently, the window icon is the image with the url set through the 'icon' property from the CreateWindowOptions. A default app icon badge will be added to this window icon.
https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... File extensions/browser/app_window/app_window.h (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... extensions/browser/app_window/app_window.h:593: gfx::Image window_icon_; Why is this a separate field from app_icon_ if we never show app_icon_ when window_icon_url_ is provided?
https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:92: return; nit: I would use an else here instead of an early exit since the two conditions are doing mostly the same thing. https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:196: // Restore any existing app icon and flag as set. This is confusing here. I would put both comments inside each if/else clause. https://codereview.chromium.org/2209053004/diff/1/extensions/browser/api/app_... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/api/app_... extensions/browser/api/app_window/app_window_api.cc:340: extension()->GetResourceURL(*options->icon.get()); {} https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... File extensions/browser/app_window/app_window.h (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... extensions/browser/app_window/app_window.h:593: gfx::Image window_icon_; On 2016/08/04 18:02:42, Reilly Grant wrote: > Why is this a separate field from app_icon_ if we never show app_icon_ when > window_icon_url_ is provided? I tend to agree. If we use a single icon image and put the logic in AppWindow to set its icon appropriately, we can remove that logic elsewhere. https://codereview.chromium.org/2209053004/diff/1/extensions/common/api/app_w... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/2209053004/diff/1/extensions/common/api/app_w... extensions/common/api/app_window.idl:234: // URL of the window icon. An window can have its own icon when showInShelf 'A window'
Thank you for review, Reilly and Steven. Uploaded a new patch set that covers the review and added the badge icon to the showInShelf=true windows icon. https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:92: return; On 2016/08/04 21:00:47, stevenjb wrote: > nit: I would use an else here instead of an early exit since the two conditions > are doing mostly the same thing. Done. https://codereview.chromium.org/2209053004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:196: // Restore any existing app icon and flag as set. On 2016/08/04 21:00:47, stevenjb wrote: > This is confusing here. I would put both comments inside each if/else clause. Done. https://codereview.chromium.org/2209053004/diff/1/extensions/browser/api/app_... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/api/app_... extensions/browser/api/app_window/app_window_api.cc:340: extension()->GetResourceURL(*options->icon.get()); On 2016/08/04 21:00:47, stevenjb wrote: > {} Done. https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... File extensions/browser/app_window/app_window.h (right): https://codereview.chromium.org/2209053004/diff/1/extensions/browser/app_wind... extensions/browser/app_window/app_window.h:593: gfx::Image window_icon_; On 2016/08/04 21:00:47, stevenjb wrote: > On 2016/08/04 18:02:42, Reilly Grant wrote: > > Why is this a separate field from app_icon_ if we never show app_icon_ when > > window_icon_url_ is provided? > > I tend to agree. If we use a single icon image and put the logic in AppWindow to > set its icon appropriately, we can remove that logic elsewhere. Thank you, Reilly and Steven, for feedback. I updated the CL to use only app_icon, without a separate window_icon field. Then we could use the app_icon_image variable, instead of app_icon, to set the default app icon badge of the showInShelf=true windows. https://codereview.chromium.org/2209053004/diff/1/extensions/common/api/app_w... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/2209053004/diff/1/extensions/common/api/app_w... extensions/common/api/app_window.idl:234: // URL of the window icon. An window can have its own icon when showInShelf On 2016/08/04 21:00:47, stevenjb wrote: > 'A window' Done.
https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/api/... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/api/... extensions/browser/api/app_window/app_window_api.cc:337: If I read this correctly, first we are looking for a valid global URL, then a valid extension local URL? We should add a comment here. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:315: window_icon_url_, We should test that window_icon_url_ is valid first. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:619: if (!window_icon_url_.is_empty()) { We should test that app_icon_image_ is valid. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:835: return; {} https://codereview.chromium.org/2209053004/diff/20001/extensions/common/api/a... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/common/api/a... extensions/common/api/app_window.idl:235: // is set to true. We should document what types of urls are valid here. https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:577: return ImageSkia(); It seems reasonable that if 'first' (the icon) is valid but second is not, we should return 'first' instead of an empty image? Either way we should document the behavior in the header. https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.h (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.h:99: const ImageSkia& second); Instead of 'first' and 'second', use 'image' and 'badge', and maybe we should replace 'image' with icon' so this would be CreateIconWithBadge(icon, badge).
Thank you for feedback, Steven. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/api/... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/api/... extensions/browser/api/app_window/app_window_api.cc:337: On 2016/08/08 22:26:45, stevenjb wrote: > If I read this correctly, first we are looking for a valid global URL, then a > valid extension local URL? We should add a comment here. Done. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:315: window_icon_url_, On 2016/08/08 22:26:46, stevenjb wrote: > We should test that window_icon_url_ is valid first. Done. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:619: if (!window_icon_url_.is_empty()) { On 2016/08/08 22:26:45, stevenjb wrote: > We should test that app_icon_image_ is valid. Done. https://codereview.chromium.org/2209053004/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:835: return; On 2016/08/08 22:26:46, stevenjb wrote: > {} Done. https://codereview.chromium.org/2209053004/diff/20001/extensions/common/api/a... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/2209053004/diff/20001/extensions/common/api/a... extensions/common/api/app_window.idl:235: // is set to true. On 2016/08/08 22:26:46, stevenjb wrote: > We should document what types of urls are valid here. Done. https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:577: return ImageSkia(); On 2016/08/08 22:26:46, stevenjb wrote: > It seems reasonable that if 'first' (the icon) is valid but second is not, we > should return 'first' instead of an empty image? Either way we should document > the behavior in the header. > Yes, we could do this way. Also, should we return the badge icon as a default icon if the first icon is not valid? Added this logic in the new patch set. https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.h (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.h:99: const ImageSkia& second); On 2016/08/08 22:26:46, stevenjb wrote: > Instead of 'first' and 'second', use 'image' and 'badge', and maybe we should > replace 'image' with icon' so this would be CreateIconWithBadge(icon, badge). > > Done.
https://codereview.chromium.org/2209053004/diff/60001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.h (right): https://codereview.chromium.org/2209053004/diff/60001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.h:99: // the badge is valid and the icon is not, the badge icon will be returned. This seems overly complex. An empty |badge| seems reasonable, but if |icon| is invalid we should just return an empty image.
https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:577: return ImageSkia(); On 2016/08/09 at 11:34:47, andra.paraschiv wrote: > On 2016/08/08 22:26:46, stevenjb wrote: > > It seems reasonable that if 'first' (the icon) is valid but second is not, we > > should return 'first' instead of an empty image? Either way we should document > > the behavior in the header. > > > > Yes, we could do this way. Also, should we return the badge icon as a default icon if the first icon is not valid? Added this logic in the new patch set. If the icon is not yet available we should use the default placeholder app icon and badge it with the icon of the app that created the window.
Thank you for feedback. https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/20001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:577: return ImageSkia(); On 2016/08/09 19:08:45, Reilly Grant wrote: > On 2016/08/09 at 11:34:47, andra.paraschiv wrote: > > On 2016/08/08 22:26:46, stevenjb wrote: > > > It seems reasonable that if 'first' (the icon) is valid but second is not, > we > > > should return 'first' instead of an empty image? Either way we should > document > > > the behavior in the header. > > > > > > > Yes, we could do this way. Also, should we return the badge icon as a default > icon if the first icon is not valid? Added this logic in the new patch set. > > If the icon is not yet available we should use the default placeholder app icon > and badge it with the icon of the app that created the window. Done. https://codereview.chromium.org/2209053004/diff/60001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.h (right): https://codereview.chromium.org/2209053004/diff/60001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.h:99: // the badge is valid and the icon is not, the badge icon will be returned. On 2016/08/09 16:59:32, stevenjb wrote: > This seems overly complex. An empty |badge| seems reasonable, but if |icon| is > invalid we should just return an empty image. Done.
https://codereview.chromium.org/2209053004/diff/80001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/2209053004/diff/80001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:626: if (app_icon_.IsEmpty()) { This will never happen since we already checked that image is not empty above. I beleive the logic should just be be: base_image = !image.IsEmtpy() ? image : ...GetImageSkiaNamed(IDR_APP_DEFAULT_ICON); app_icon_ = ...CreateIconWithBadge(base_image, app_icon_image_); ? https://codereview.chromium.org/2209053004/diff/80001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/80001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:579: if (!icon.isNull() && badge.isNull()) This can just be if (badge.isNull())
https://codereview.chromium.org/2209053004/diff/80001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/2209053004/diff/80001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:626: if (app_icon_.IsEmpty()) { On 2016/08/10 16:58:47, stevenjb wrote: > This will never happen since we already checked that image is not empty above. > > I beleive the logic should just be be: > > base_image = !image.IsEmtpy() ? image : > ...GetImageSkiaNamed(IDR_APP_DEFAULT_ICON); > app_icon_ = ...CreateIconWithBadge(base_image, app_icon_image_); > > ? Right, thank you for this, updated the CL. https://codereview.chromium.org/2209053004/diff/80001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/2209053004/diff/80001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:579: if (!icon.isNull() && badge.isNull()) On 2016/08/10 16:58:48, stevenjb wrote: > This can just be if (badge.isNull()) Done.
lgtm
On 2016/08/11 19:04:15, stevenjb wrote: > lgtm Also, we really need tests for this. Unfortunately there is not currently good test coverage for AppWindow, but even so we should really add a test in AppWindowBrowserTest that sets show_in_shelf and window_icon_url and verifies that the correct icon is generated. This will not be a simple test to write and unfortunately I don't have a lot of time to help look for examples, but there may be some similar tests in AppWindow derived classes or other areas of the browser. We can land this as-is, but we should make sure to have good test coverage of the feature before it ships.
lgtm
The CQ bit was checked by andra.paraschiv@intel.com
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 andra.paraschiv@intel.com
On 2016/08/11 19:15:04, stevenjb wrote: > On 2016/08/11 19:04:15, stevenjb wrote: > > lgtm > > Also, we really need tests for this. Unfortunately there is not currently good > test coverage for AppWindow, but even so we should really add a test in > AppWindowBrowserTest that sets show_in_shelf and window_icon_url and verifies > that the correct icon is generated. This will not be a simple test to write and > unfortunately I don't have a lot of time to help look for examples, but there > may be some similar tests in AppWindow derived classes or other areas of the > browser. > > We can land this as-is, but we should make sure to have good test coverage of > the feature before it ships. Indeed we need more tests for the feature, we will look through the source code to see if there are other examples for icon setting and try to get it done and uploaded through another CL. Thank you both for the review.
The CQ bit was checked by andra.paraschiv@intel.com
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
andra.paraschiv@intel.com changed reviewers: + oshima@chromium.org
On 2016/08/12 07:21:58, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Added oshima as reviewer. Could you please take a look?
lgtm
lgtm
The CQ bit was checked by andra.paraschiv@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. ========== to ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. ========== to ========== Enhance chrome.app.window API with icon property Enhance chrome.app.window API with the possibility of creating a window that has its own icon in the shelf. Added a new property, icon, to CreateWindowOptions. Based on https://codereview.chromium.org/1914993002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with icon property set to an image url. Observe that the newly created windows have their own icons in the shelf. Committed: https://crrev.com/ebbb79d24ceb6625788dc9b121b880700b6340d6 Cr-Commit-Position: refs/heads/master@{#412185} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ebbb79d24ceb6625788dc9b121b880700b6340d6 Cr-Commit-Position: refs/heads/master@{#412185} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
