Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1914993002:
Enhance chrome.app.window API with better shelf integration (Closed)
|
Created:
4 years, 8 months ago by Valentin Ilie Modified:
4 years, 5 months ago CC:
chromium-reviews 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 better shelf integration
Enhance chrome.app.window API with the possibility of creating a window that
will show up separately in the shelf, tied to its own icon.
Added a new property, showInShelf, to CreateWindowOptions. Default value for
the property is false.
Based on https://codereview.chromium.org/1811523002
Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com>;
Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com>;
BUG=610299
TEST=interactive_ui_tests
Implement a simple extension that creates windows with the property set
to true. Observe that the newly created windows are added as separate icons in
the shelf.
Committed: https://crrev.com/3049b971f57ee97b1a602605e90ca18223a30653
Cr-Commit-Position: refs/heads/master@{#405978}
Patch Set 1 #Patch Set 2 : browsertest: Close windows in other order #
Total comments: 30
Patch Set 3 : Review v1 #Patch Set 4 : Rebase v1 #
Total comments: 10
Patch Set 5 : Review v2 #Patch Set 6 : Rebase + Fixes #
Total comments: 22
Patch Set 7 : Rebase + Fixes v4 #
Total comments: 18
Patch Set 8 : Rebase + Fixes v5 #
Total comments: 3
Patch Set 9 : Rebase + Fixes v6 #
Total comments: 2
Patch Set 10 : Rebase + Fixes v7 #Patch Set 11 : Rebase + Fixes v8 #Patch Set 12 : Fix LaunchPinned/UnpinRunning v9 #
Total comments: 4
Patch Set 13 : Rebase + Fixes v10 #Messages
Total messages: 59 (18 generated)
valentin.ilie@intel.com changed reviewers: + stevenjb@chromium.org
This is based on https://codereview.chromium.org/1811523002; Continued the work + some logic fixes.
On 2016/04/25 13:01:06, Valentin Ilie wrote: > This is based on https://codereview.chromium.org/1811523002; Continued the work > + some logic fixes. @stevenjb Could you take a look?
https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1498: // Test 1 Describe what each part of this test is testing for instead of 'Test 1', etc. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1506: item_count++; nit: instead of incrementing item_count here and below, this would be a little more readable as: EXPECT_EQ(item_count + 1, ...) https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1549: item_count += 1; This seems incorrect. In the second test, 2 items are created. The creation order shouldn't matter. window 2 should be associated with the "default" icon and window 1 should have a separate item. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1551: CloseAppWindow(window1); We should also test that the shelf item(s) go away when they are supposed to. e.g. after every CloseAppWindow we should test item_count. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1568: // There should be 3 item added to the shelf. s/item/items/ https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:161: app_controller_map_[app_shelf_id] = controller; Use an iterator instead of doing the map lookup twice. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:163: window_controller_map_[window] = controller; This will only contain entries for secondary windows. We should name the member to reflect that and explain that in the header comment. Also, this logic seems incomplete. If the first window has show_in_shelf = true, and another window is added with show_in_shelf = false, no new icon will be created, which seems incorrect. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:179: WindowControllerMap::iterator iter3 = window_controller_map_.find(window); 'iter2' wasn't a good name and adding 'iter3' makes it worse. Can we rename these something like 'shelf_id_iter', 'app_controller_iter', and 'secondary_window_iter'? https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:217: AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id); In this case we don't need iter2 unless iter3 == end. Move this line below 220 and rename the iterators. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:80: WindowControllerMap window_controller_map_; Comment this (and maybe place below window_to_app_shelf_id_map_ to match typedef order. https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:300: show_in_shelf_ = params.show_in_shelf; nit: no blank lines between these three lines. https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.h (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.h:195: // being folded in the menu of the parent app. Defaults to false. "folded in the menu of the parent app" isn't clear. If all windows have show_in_shelf = true, is there a "parent app menu"? I think the logic would be more clear as: "If true, the window will have its own shelf icon. Otherwise the window will be grouped in the shelf with other windows associated with the app." https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.h:375: // Whether the app window will create its own icon in the shelf. Trivial getters shouldn't have comments, instead the variable itself should be commented (which it is). You can also remove he comment for requested_alpha_enabled() (but leave the TODO for is_ime_window(), that should be addressed separately). https://codereview.chromium.org/1914993002/diff/20001/extensions/common/api/a... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/common/api/a... extensions/common/api/app_window.idl:229: // folded in the menu of the parent app. Defaults to false. Use the comment I suggested previously (or something similar) here also.
We addressed most of the feedback. Do you have any information about how this CL might interfere with https://codereview.chromium.org/1896223002 ? Both CLs have close goals, the only difference is that #1896223002 only works for ARC apps. Could we clarify the expected behavior for opening the first window with showInShelf true? As the first window always has an icon on the shelf I'm not sure that the upcoming window that has showInShelf false should also have a separate icon entry.
1. Please respond to each review comment with 'Done' or a question or comment. This makes reviewing the changes easier. 2. Arc apps are separate from extension apps so the CLs do not seem closely related. You will need to rebase/merge with that CL of course. We should also add skuhne@ as a reviewer since he is more recently familiar with the code than I am. 3. We should definitely clarify the expected behavior, but having the order in which windows are opened affect the shelf icon state seems untenable to me. Grouping all windows with showInShelf=false separately seems like the only viable option, but I am happy to discuss that. On Wed, Apr 27, 2016 at 8:20 AM, <valentin.ilie@intel.com> wrote: > We addressed most of the feedback. > > Do you have any information about how this CL might interfere with > https://codereview.chromium.org/1896223002 ? > > Both CLs have close goals, the only difference is that #1896223002 only > works > for ARC apps. > > Could we clarify the expected behavior for opening the first window with > showInShelf true? As the first window always has an icon on the shelf I'm > not > sure that the upcoming window that has showInShelf false should also have a > separate icon entry. > > > > https://codereview.chromium.org/1914993002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
stevenjb@chromium.org changed reviewers: + skuhne@chromium.org
+skuhne@
https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1498: // Test 1 On 2016/04/26 16:30:00, stevenjb wrote: > Describe what each part of this test is testing for instead of 'Test 1', etc. Done. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1506: item_count++; On 2016/04/26 16:30:00, stevenjb wrote: > nit: instead of incrementing item_count here and below, this would be a little > more readable as: EXPECT_EQ(item_count + 1, ...) Done. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1549: item_count += 1; On 2016/04/26 16:30:00, stevenjb wrote: > This seems incorrect. In the second test, 2 items are created. The creation > order shouldn't matter. window 2 should be associated with the "default" icon > and window 1 should have a separate item. We should decide the appropriate behavior here. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1551: CloseAppWindow(window1); On 2016/04/26 16:30:00, stevenjb wrote: > We should also test that the shelf item(s) go away when they are supposed to. > e.g. after every CloseAppWindow we should test item_count. Done. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1568: // There should be 3 item added to the shelf. On 2016/04/26 16:30:00, stevenjb wrote: > s/item/items/ Done.
https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:161: app_controller_map_[app_shelf_id] = controller; On 2016/04/26 16:30:00, stevenjb wrote: > Use an iterator instead of doing the map lookup twice. The code remained the same. When using an iterator the end of the map is reached after calling the find method "app_controller_map_->find(app_shelf_id)". https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:163: window_controller_map_[window] = controller; On 2016/04/26 16:30:00, stevenjb wrote: > This will only contain entries for secondary windows. We should name the member > to reflect that and explain that in the header comment. > > Also, this logic seems incomplete. If the first window has show_in_shelf = true, > and another window is added with show_in_shelf = false, no new icon will be > created, which seems incorrect. Renamed the member to secondary_window_controller_map. Regarding the logic we should clarify it (e.g. windows with show_in_shelf=true/false have or not their icon in the shelf depending on the order in which are opened) before modifying the code. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:179: WindowControllerMap::iterator iter3 = window_controller_map_.find(window); On 2016/04/26 16:30:00, stevenjb wrote: > 'iter2' wasn't a good name and adding 'iter3' makes it worse. Can we rename > these something like 'shelf_id_iter', 'app_controller_iter', and > 'secondary_window_iter'? Done. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:217: AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id); On 2016/04/26 16:30:00, stevenjb wrote: > In this case we don't need iter2 unless iter3 == end. Move this line below 220 > and rename the iterators. Done. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:80: WindowControllerMap window_controller_map_; On 2016/04/26 16:30:00, stevenjb wrote: > Comment this (and maybe place below window_to_app_shelf_id_map_ to match typedef > order. Done. https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.cc (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.cc:300: show_in_shelf_ = params.show_in_shelf; On 2016/04/26 16:30:00, stevenjb wrote: > nit: no blank lines between these three lines. Done. https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... File extensions/browser/app_window/app_window.h (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.h:195: // being folded in the menu of the parent app. Defaults to false. On 2016/04/26 16:30:01, stevenjb wrote: > "folded in the menu of the parent app" isn't clear. If all windows have > show_in_shelf = true, is there a "parent app menu"? I think the logic would be > more clear as: > > "If true, the window will have its own shelf icon. Otherwise the window will be > grouped in the shelf with other windows associated with the app." Done. https://codereview.chromium.org/1914993002/diff/20001/extensions/browser/app_... extensions/browser/app_window/app_window.h:375: // Whether the app window will create its own icon in the shelf. On 2016/04/26 16:30:01, stevenjb wrote: > Trivial getters shouldn't have comments, instead the variable itself should be > commented (which it is). You can also remove he comment for > requested_alpha_enabled() (but leave the TODO for is_ime_window(), that should > be addressed separately). > Done. https://codereview.chromium.org/1914993002/diff/20001/extensions/common/api/a... File extensions/common/api/app_window.idl (right): https://codereview.chromium.org/1914993002/diff/20001/extensions/common/api/a... extensions/common/api/app_window.idl:229: // folded in the menu of the parent app. Defaults to false. On 2016/04/26 16:30:01, stevenjb wrote: > Use the comment I suggested previously (or something similar) here also. Done.
Mostly nits, + the concerns previously mentioned which we can discuss in the design doc or in a meeting with a broader group. https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:161: app_controller_map_[app_shelf_id] = controller; Actually, [] will insert a new element (so this effectively inserts a default (null) element then inserts |controller| over it), but frankly we shouldn't be relying on that anyway. Instead use: if (!ContainsKey(app_controller_map_, app_shelf_id)) (ContainsKey is a helper in stl_util.h). https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1552: // is set to show_in_shelf false This comment should be above the EXPECT https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1573: EXPECT_EQ(item_count + 3, shelf_model->item_count()); I think we should have an EXPECT for each Create here also. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1576: EXPECT_EQ(item_count + 2, shelf_model->item_count()); I also think each of these merits an explanation since the behavior is not entirely trivial. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:218: std::string app_shelf_id = shelf_id_iter->second; No need for this temp, just use shelf_id_iter->second below (which is more clear now that it is better named). https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:84: // Map of secondary window to controller. The secondary window has its own s/The/Each/
Also, this needs a link to a tracking issue in crbug.com (which should have a link to the design doc).
Description was changed from ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG= TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ========== to ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=CRBUG:583639 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ==========
Description was changed from ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=CRBUG:583639 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ========== to ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=583639 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ==========
https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:161: app_controller_map_[app_shelf_id] = controller; On 2016/05/03 20:39:48, stevenjb wrote: > Actually, [] will insert a new element (so this effectively inserts a default > (null) element then inserts |controller| over it), but frankly we shouldn't be > relying on that anyway. Instead use: > > if (!ContainsKey(app_controller_map_, app_shelf_id)) > > (ContainsKey is a helper in stl_util.h). Done. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1552: // is set to show_in_shelf false On 2016/05/03 20:39:49, stevenjb wrote: > This comment should be above the EXPECT Done. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1573: EXPECT_EQ(item_count + 3, shelf_model->item_count()); On 2016/05/03 20:39:49, stevenjb wrote: > I think we should have an EXPECT for each Create here also. Done. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1576: EXPECT_EQ(item_count + 2, shelf_model->item_count()); On 2016/05/03 20:39:48, stevenjb wrote: > I also think each of these merits an explanation since the behavior is not > entirely trivial. Done. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:218: std::string app_shelf_id = shelf_id_iter->second; On 2016/05/03 20:39:49, stevenjb wrote: > No need for this temp, just use shelf_id_iter->second below (which is more clear > now that it is better named). Done. https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h (right): https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:84: // Map of secondary window to controller. The secondary window has its own On 2016/05/03 20:39:49, stevenjb wrote: > s/The/Each/ Done.
Description was changed from ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=583639 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ========== to ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ==========
On 2016/05/04 11:59:09, andra.paraschiv wrote: > https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... > File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > (right): > > https://codereview.chromium.org/1914993002/diff/20001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:161: > app_controller_map_[app_shelf_id] = controller; > On 2016/05/03 20:39:48, stevenjb wrote: > > Actually, [] will insert a new element (so this effectively inserts a default > > (null) element then inserts |controller| over it), but frankly we shouldn't be > > relying on that anyway. Instead use: > > > > if (!ContainsKey(app_controller_map_, app_shelf_id)) > > > > (ContainsKey is a helper in stl_util.h). > > Done. > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc > (right): > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1552: > // is set to show_in_shelf false > On 2016/05/03 20:39:49, stevenjb wrote: > > This comment should be above the EXPECT > > Done. > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1573: > EXPECT_EQ(item_count + 3, shelf_model->item_count()); > On 2016/05/03 20:39:49, stevenjb wrote: > > I think we should have an EXPECT for each Create here also. > > Done. > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1576: > EXPECT_EQ(item_count + 2, shelf_model->item_count()); > On 2016/05/03 20:39:48, stevenjb wrote: > > I also think each of these merits an explanation since the behavior is not > > entirely trivial. > > Done. > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > (right): > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:218: > std::string app_shelf_id = shelf_id_iter->second; > On 2016/05/03 20:39:49, stevenjb wrote: > > No need for this temp, just use shelf_id_iter->second below (which is more > clear > > now that it is better named). > > Done. > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h > (right): > > https://codereview.chromium.org/1914993002/diff/60001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:84: // > Map of secondary window to controller. The secondary window has its own > On 2016/05/03 20:39:49, stevenjb wrote: > > s/The/Each/ > > Done. We updated the CL for the #1 requirement. Could you please take a look?
Apologies, I'm kind of swamped and couldn't do a better job of making suggestions, but I would really like to find a way to do this with a bit less complexity if possible. If you're not sure what I mean I will try to make some more time for this later in the week, or early next week. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:1735: } This needs to be broken up https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:145: // case when the showInShelf parameter is true. This is confusing. We should have a comment in the new if() clause describing its behavior, and the old comment (updated) in the else() clause. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:147: ContainsKey(window_id_to_shelf_id_map_, window_key)) { This should be window_key_to_shelf_id_map_. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:148: shelf_id = window_id_to_shelf_id_map_[window_key]; We are doing two lookups. We should get an iterator to window_key_to_shelf_id_map_ instead of using COntainsKey and use that here. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:149: if (!owner()->IsPinned(shelf_id)) { I'm not following what this is supposed to do. Why are we setting shelf_id to 0 of there is an existing unpinned item with the same key? https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:154: else if (app_shelf_id == app_id && !app_window->show_in_shelf() && else on previous line https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:156: window_id_to_shelf_id_map_.empty()) { Why test for an empty map? Couldn't a different app could have entries? https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:167: owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia()); Currently the extra window icons will still have the app icon image, right? I think we need to have a separate CL that allows chrome.app.window.create to specify the icon first, so that secondary icons can have a distinct icon. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:186: } Having 3 different maps in this code now is super confusing. Could we do something like this: window_key = !app_window->window_key().empty() ? window_key = app_window->window_key() : app_id; Then just have one map of window_key -> shelf item? https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:204: secondary_window_iter != secondary_window_controller_map_.end()); I think we should be able to combine these maps. window_to_app_shelf_id_map_ should have the correct id for the window, whether it is a secondary window or not. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:224: } I think we ma have an stl util helper to do this if we still need it.
https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:167: owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia()); On 2016/06/14 18:33:21, stevenjb wrote: > Currently the extra window icons will still have the app icon image, right? > > I think we need to have a separate CL that allows chrome.app.window.create to > specify the icon first, so that secondary icons can have a distinct icon. Yes, extra window icons still have the app icon image, we need to add the icon parameter to the window create options. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:186: } On 2016/06/14 18:33:21, stevenjb wrote: > Having 3 different maps in this code now is super confusing. > > Could we do something like this: > > window_key = !app_window->window_key().empty() ? window_key = > app_window->window_key() : app_id; > > Then just have one map of window_key -> shelf item? We could use the window key to map to the shelf item for windows with showInShelf parameter set to true or false. Can we use app_shelf_id (as it is now in the default behavior for showInShelf=false) for all windows with showInShelf=false and the window_key value for windows with showInShelf=true? There could be two possible solutions: 1. We use the window_key value for all the windows that have it set. If the window_key is not set, we use the app_shelf_id value. window_key = !app_window->window_key().empty() ? app_window->window_key() : app_shelf_id; In this case e.g. a window with showInShelf=true and one with showInShelf=false could not have the window_key set. We use the app_shelf_id value as the window_key for both. Each of them will have a different shelf_id and we will use the same app_shelf_id map key for 2 different shelf_id values. 2. All showInShelf=true windows should have a window_key set and we use this value for the map. For the showInShelf=false windows we always use the app_shelf_id regardless of the value of their window_key. window_key = app_shelf_id; if (!app_window->window_key().empty() && app_window->showInShelf()) window_key = app_window->window_key();
On 2016/06/21 09:04:14, andra.paraschiv wrote: > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > (right): > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:167: > owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia()); > On 2016/06/14 18:33:21, stevenjb wrote: > > Currently the extra window icons will still have the app icon image, right? > > > > I think we need to have a separate CL that allows chrome.app.window.create to > > specify the icon first, so that secondary icons can have a distinct icon. > > Yes, extra window icons still have the app icon image, we need to add the icon > parameter to the window create options. > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:186: > } > On 2016/06/14 18:33:21, stevenjb wrote: > > Having 3 different maps in this code now is super confusing. > > > > Could we do something like this: > > > > window_key = !app_window->window_key().empty() ? window_key = > > app_window->window_key() : app_id; > > > > Then just have one map of window_key -> shelf item? > > We could use the window key to map to the shelf item for windows with > showInShelf parameter set to true or false. > > Can we use app_shelf_id (as it is now in the default behavior for > showInShelf=false) for all windows with showInShelf=false and the window_key > value for windows with showInShelf=true? > > There could be two possible solutions: > > 1. We use the window_key value for all the windows that have it set. If the > window_key is not set, we use the app_shelf_id value. > > window_key = !app_window->window_key().empty() ? app_window->window_key() : > app_shelf_id; > > In this case e.g. a window with showInShelf=true and one with showInShelf=false > could not have the window_key set. We use the app_shelf_id value as the > window_key for both. Each of them will have a different shelf_id and we will use > the same app_shelf_id map key for 2 different shelf_id values. > > 2. All showInShelf=true windows should have a window_key set and we use this > value for the map. For the showInShelf=false windows we always use the > app_shelf_id regardless of the value of their window_key. > > window_key = app_shelf_id; > if (!app_window->window_key().empty() && app_window->showInShelf()) > window_key = app_window->window_key(); What if we go with a variation of #2, where we ensure that all windows with show_in_shelf == true have a window key: window_key = app_window->show_in_shelf() ? app_window->EnsureWindowKey() : app_shelf_id; const std::string& AppWindow::EnsureWindowKey() { if (window_key_.empty()) window_key_ = base::GenerateGUID(); return window_key_; } Alternatively we could set a generated window_key when we create the window if not provided by the app in AppWindow::Init when show_in_shelf is true.
On 2016/06/21 18:18:21, stevenjb wrote: > On 2016/06/21 09:04:14, andra.paraschiv wrote: > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > File > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > > (right): > > > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:167: > > owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia()); > > On 2016/06/14 18:33:21, stevenjb wrote: > > > Currently the extra window icons will still have the app icon image, right? > > > > > > I think we need to have a separate CL that allows chrome.app.window.create > to > > > specify the icon first, so that secondary icons can have a distinct icon. > > > > Yes, extra window icons still have the app icon image, we need to add the icon > > parameter to the window create options. > > > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:186: > > } > > On 2016/06/14 18:33:21, stevenjb wrote: > > > Having 3 different maps in this code now is super confusing. > > > > > > Could we do something like this: > > > > > > window_key = !app_window->window_key().empty() ? window_key = > > > app_window->window_key() : app_id; > > > > > > Then just have one map of window_key -> shelf item? > > > > We could use the window key to map to the shelf item for windows with > > showInShelf parameter set to true or false. > > > > Can we use app_shelf_id (as it is now in the default behavior for > > showInShelf=false) for all windows with showInShelf=false and the window_key > > value for windows with showInShelf=true? > > > > There could be two possible solutions: > > > > 1. We use the window_key value for all the windows that have it set. If the > > window_key is not set, we use the app_shelf_id value. > > > > window_key = !app_window->window_key().empty() ? app_window->window_key() : > > app_shelf_id; > > > > In this case e.g. a window with showInShelf=true and one with > showInShelf=false > > could not have the window_key set. We use the app_shelf_id value as the > > window_key for both. Each of them will have a different shelf_id and we will > use > > the same app_shelf_id map key for 2 different shelf_id values. > > > > 2. All showInShelf=true windows should have a window_key set and we use this > > value for the map. For the showInShelf=false windows we always use the > > app_shelf_id regardless of the value of their window_key. > > > > window_key = app_shelf_id; > > if (!app_window->window_key().empty() && app_window->showInShelf()) > > window_key = app_window->window_key(); > > What if we go with a variation of #2, where we ensure that all windows with > show_in_shelf == true have a window key: > > window_key = app_window->show_in_shelf() ? app_window->EnsureWindowKey() : > app_shelf_id; > > const std::string& AppWindow::EnsureWindowKey() { > if (window_key_.empty()) > window_key_ = base::GenerateGUID(); > return window_key_; > } > > Alternatively we could set a generated window_key when we create the window if > not provided by the app in AppWindow::Init when show_in_shelf is true. Sure, Steven, we can go with this, but I have one question. Is the window_key value the same when we generate a guid, then close and open the same window (e.g. when pinned) with window_key not set in the javascript file? For example we open a window with no id set in the .js file (from CreateWindowOptions). We generate the guid and we map it to the shelf id. We pin the showInShelf=true window and then close it. When we open the same window again, it will not have the id set in the .js file. Will we generate a new guid? We should use the first generated id to get the shelf id from the map. Thanks!
Yes, that would be a problem, thank you for thinking that through. In practice, it doesn't really make sense to pin a window that doesn't have a key provided to the app, right? There would be no way to tell the app which window to open. We could attempt to disallow pinning in that case, but that would add some complexity to the shelf code. Perhaps we should require window_key to be set with show_in_shelf ? On Thu, Jun 23, 2016 at 2:23 AM, <andra.paraschiv@intel.com> wrote: > On 2016/06/21 18:18:21, stevenjb wrote: > > On 2016/06/21 09:04:14, andra.paraschiv wrote: > > > > > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > > File > > > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > > > (right): > > > > > > > > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > > > > > > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:167: > > > owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia()); > > > On 2016/06/14 18:33:21, stevenjb wrote: > > > > Currently the extra window icons will still have the app icon image, > right? > > > > > > > > I think we need to have a separate CL that allows > chrome.app.window.create > > to > > > > specify the icon first, so that secondary icons can have a distinct > icon. > > > > > > Yes, extra window icons still have the app icon image, we need to add > the > icon > > > parameter to the window create options. > > > > > > > > > > https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... > > > > > > > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:186: > > > } > > > On 2016/06/14 18:33:21, stevenjb wrote: > > > > Having 3 different maps in this code now is super confusing. > > > > > > > > Could we do something like this: > > > > > > > > window_key = !app_window->window_key().empty() ? window_key = > > > > app_window->window_key() : app_id; > > > > > > > > Then just have one map of window_key -> shelf item? > > > > > > We could use the window key to map to the shelf item for windows with > > > showInShelf parameter set to true or false. > > > > > > Can we use app_shelf_id (as it is now in the default behavior for > > > showInShelf=false) for all windows with showInShelf=false and the > window_key > > > value for windows with showInShelf=true? > > > > > > There could be two possible solutions: > > > > > > 1. We use the window_key value for all the windows that have it set. > If the > > > window_key is not set, we use the app_shelf_id value. > > > > > > window_key = !app_window->window_key().empty() ? > app_window->window_key() : > > > app_shelf_id; > > > > > > In this case e.g. a window with showInShelf=true and one with > > showInShelf=false > > > could not have the window_key set. We use the app_shelf_id value as the > > > window_key for both. Each of them will have a different shelf_id and > we will > > use > > > the same app_shelf_id map key for 2 different shelf_id values. > > > > > > 2. All showInShelf=true windows should have a window_key set and we > use this > > > value for the map. For the showInShelf=false windows we always use the > > > app_shelf_id regardless of the value of their window_key. > > > > > > window_key = app_shelf_id; > > > if (!app_window->window_key().empty() && app_window->showInShelf()) > > > window_key = app_window->window_key(); > > > > What if we go with a variation of #2, where we ensure that all windows > with > > show_in_shelf == true have a window key: > > > > window_key = app_window->show_in_shelf() ? app_window->EnsureWindowKey() > : > > app_shelf_id; > > > > const std::string& AppWindow::EnsureWindowKey() { > > if (window_key_.empty()) > > window_key_ = base::GenerateGUID(); > > return window_key_; > > } > > > > Alternatively we could set a generated window_key when we create the > window if > > not provided by the app in AppWindow::Init when show_in_shelf is true. > > Sure, Steven, we can go with this, but I have one question. Is the > window_key > value the same when we generate a guid, then close and open the same window > (e.g. when pinned) with window_key not set in the javascript file? > > For example we open a window with no id set in the .js file (from > CreateWindowOptions). We generate the guid and we map it to the shelf id. > We pin > the showInShelf=true window and then close it. When we open the same window > again, it will not have the id set in the .js file. Will we generate a new > guid? > We should use the first generated id to get the shelf id from the map. > > Thanks! > > https://codereview.chromium.org/1914993002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, you are right, in order to identify which pinned window to open we need a key. For showInShelf windows we have to ensure they have a key set. Should we specify this requirement in the app window idl? Or check this condition in the AppWindow Init function? Thanks!
We can add a comment in the idl, and we can enforce it in the API by clearing showInShelf if windowKey is provided and setting an error, e.g. 'showInShelf requires windowKey to be set'. On Fri, Jun 24, 2016 at 6:50 AM, <andra.paraschiv@intel.com> wrote: > Yes, you are right, in order to identify which pinned window to open we > need a > key. > > For showInShelf windows we have to ensure they have a key set. Should we > specify > this requirement in the app window idl? Or check this condition in the > AppWindow > Init function? > > Thanks! > > https://codereview.chromium.org/1914993002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:1735: } On 2016/06/14 18:33:21, stevenjb wrote: > This needs to be broken up Done. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:145: // case when the showInShelf parameter is true. On 2016/06/14 18:33:21, stevenjb wrote: > This is confusing. We should have a comment in the new if() clause describing > its behavior, and the old comment (updated) in the else() clause. Done. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:147: ContainsKey(window_id_to_shelf_id_map_, window_key)) { On 2016/06/14 18:33:21, stevenjb wrote: > This should be window_key_to_shelf_id_map_. Done. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:148: shelf_id = window_id_to_shelf_id_map_[window_key]; On 2016/06/14 18:33:21, stevenjb wrote: > We are doing two lookups. We should get an iterator to > window_key_to_shelf_id_map_ instead of using COntainsKey and use that here. Done. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:149: if (!owner()->IsPinned(shelf_id)) { On 2016/06/14 18:33:21, stevenjb wrote: > I'm not following what this is supposed to do. Why are we setting shelf_id to 0 > of there is an existing unpinned item with the same key? If a pinned window is closed, then unpinned and opened again we should use a new shelf id, not the old one. We delete from the map the shelf id corresponding to this window key. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:154: else if (app_shelf_id == app_id && !app_window->show_in_shelf() && On 2016/06/14 18:33:21, stevenjb wrote: > else on previous line Done. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:156: window_id_to_shelf_id_map_.empty()) { On 2016/06/14 18:33:21, stevenjb wrote: > Why test for an empty map? Couldn't a different app could have entries? Yes, you are right, thank you for this notice. The new patch set doesn't include these conditions anymore. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:204: secondary_window_iter != secondary_window_controller_map_.end()); On 2016/06/14 18:33:21, stevenjb wrote: > I think we should be able to combine these maps. window_to_app_shelf_id_map_ > should have the correct id for the window, whether it is a secondary window or > not. > We combined the app_controller_map and the secondary_window_map and use the window_key value for the window_to_app_shelf_id_map. https://codereview.chromium.org/1914993002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:224: } On 2016/06/14 18:33:21, stevenjb wrote: > I think we ma have an stl util helper to do this if we still need it. > We should delete the shelf_id from the map for unpinned windows that are closed.
valentin.ilie@intel.com changed reviewers: + reillyg@chromium.org
reillyg@chromium.org: Please review changes in 1914993002
I think it's confusing to start using window_key where previously this code worked on app_shell_id. These variables are still holding app shelf IDs, just sometimes those are referring to individual windows instead of a particular app. That leads me to think that maybe instead of reusing the window ID to divide an app's windows into separate shelf entries we should add a new IDL parameter. This would allow an app to also associate multiple windows with the same shelf entry which could be useful. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:120: app_window->window_key() : app_shelf_id; app_window->window_key() is controlled by the application so we should probably prefix it with the app_shelf_id so that an extension can't trick the window manager into associating its windows with another app. It might make sense then to just integrate this logic into GetAppShelfId(). https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:153: if (!owner()->IsPinned(window_key_iter->second)) { Reverse this if statement for better readability. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:184: std::string window_key = app_shelf_id_iter->second; This isn't window_key, it's app_shelf_id. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:226: return nullptr; For symmetry with the check above I would prefer: AppControllerMap::iterator app_controller_iter = app_controller_map_.find(app_shelf_id_iter->second); if (app_controller_iter == app_controller_map_.end()) return nullptr; return app_controller_iter->second; https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... extensions/browser/api/app_window/app_window_api.cc:79: "The showInShelf=true window property requires window id to be set."; "The \"showInShelf\" option requires the \"id\" option to be set." https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... extensions/browser/api/app_window/app_window_api.cc:329: if (create_params.show_in_shelf == true && Don't check booleans with == true. https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... File extensions/browser/app_window/app_window.h (left): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... extensions/browser/app_window/app_window.h:362: // Whether the app window wants to be alpha enabled. Don't delete these comments as they're unrelated to your feature.
Thanks Reilly for the careful review; I haven't been in this code in a very long time. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:120: app_window->window_key() : app_shelf_id; On 2016/07/06 19:53:00, Reilly Grant wrote: > app_window->window_key() is controlled by the application so we should probably > prefix it with the app_shelf_id so that an extension can't trick the window > manager into associating its windows with another app. It might make sense then > to just integrate this logic into GetAppShelfId(). Good catch, thanks! https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:184: std::string window_key = app_shelf_id_iter->second; On 2016/07/06 19:53:00, Reilly Grant wrote: > This isn't window_key, it's app_shelf_id. ditto https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... extensions/browser/api/app_window/app_window_api.cc:79: "The showInShelf=true window property requires window id to be set."; On 2016/07/06 19:53:00, Reilly Grant wrote: > "The \"showInShelf\" option requires the \"id\" option to be set." See my other comment, I realized this is not strictly true except for pinning purposes. That said, for simplicity's sake this is probably fine to enforce. https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... File extensions/browser/app_window/app_window.h (left): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... extensions/browser/app_window/app_window.h:362: // Whether the app window wants to be alpha enabled. On 2016/07/06 19:53:00, Reilly Grant wrote: > Don't delete these comments as they're unrelated to your feature. I may have requested that for consistency. These variables are already documented below. Trivial setters/getters shouldn't copy/paste comments. https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:37: app_window->extension_id(); This could all be simplified. Also, I believe that session_id predates window_key and should only be used for panels if window_key is not set, e.g: std::string shelf_id = app_window->extension_id(); if (app_window->show_in_shelf() || app_window->window_type_is_panel()) { if (!app_window->window_key().empty()) shelf_id += app_window->window_key(); else shelf_id += base::StringToInt(app_window->session_id().id()); } return shelf_id; Note: This would also make it OK for window_key() to be empty since session_id will always be unique per window (I had forgotten about it). We shouldn't allow items with an empty window_key/id to be pinned, but they could still have their own icon within a session without specifying 'id' in the create params.
Thank you Reilly and Steven for the review. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:120: app_window->window_key() : app_shelf_id; On 2016/07/07 22:55:54, stevenjb wrote: > On 2016/07/06 19:53:00, Reilly Grant wrote: > > app_window->window_key() is controlled by the application so we should > probably > > prefix it with the app_shelf_id so that an extension can't trick the window > > manager into associating its windows with another app. It might make sense > then > > to just integrate this logic into GetAppShelfId(). > > Good catch, thanks! Thank you Reilly for the review and for noticing this, we changed the logic to append the window_key to the app_shelf_id. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:153: if (!owner()->IsPinned(window_key_iter->second)) { On 2016/07/06 19:53:00, Reilly Grant wrote: > Reverse this if statement for better readability. Done. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:184: std::string window_key = app_shelf_id_iter->second; On 2016/07/07 22:55:54, stevenjb wrote: > On 2016/07/06 19:53:00, Reilly Grant wrote: > > This isn't window_key, it's app_shelf_id. > > ditto Done. https://codereview.chromium.org/1914993002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:226: return nullptr; On 2016/07/06 19:53:00, Reilly Grant wrote: > For symmetry with the check above I would prefer: > > AppControllerMap::iterator app_controller_iter = > app_controller_map_.find(app_shelf_id_iter->second); > if (app_controller_iter == app_controller_map_.end()) > return nullptr; > return app_controller_iter->second; Done. https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... File extensions/browser/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... extensions/browser/api/app_window/app_window_api.cc:79: "The showInShelf=true window property requires window id to be set."; On 2016/07/07 22:55:54, stevenjb wrote: > On 2016/07/06 19:53:00, Reilly Grant wrote: > > "The \"showInShelf\" option requires the \"id\" option to be set." > > See my other comment, I realized this is not strictly true except for pinning > purposes. That said, for simplicity's sake this is probably fine to enforce. Yes, as Steven mentioned before, the pinning needs an unique id for each window shown in shelf. Otherwise, if we only open the showInShelf=true window without pinning, the legacy app_shelf_id can be used. https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/api... extensions/browser/api/app_window/app_window_api.cc:329: if (create_params.show_in_shelf == true && On 2016/07/06 19:53:00, Reilly Grant wrote: > Don't check booleans with == true. Done. https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... File extensions/browser/app_window/app_window.h (left): https://codereview.chromium.org/1914993002/diff/120001/extensions/browser/app... extensions/browser/app_window/app_window.h:362: // Whether the app window wants to be alpha enabled. On 2016/07/07 22:55:54, stevenjb wrote: > On 2016/07/06 19:53:00, Reilly Grant wrote: > > Don't delete these comments as they're unrelated to your feature. > > I may have requested that for consistency. These variables are already > documented below. Trivial setters/getters shouldn't copy/paste comments. Yes, Steven, you are right regarding the request. Could you please tell us if we should we keep or not these comments in the new patch set? https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:37: app_window->extension_id(); On 2016/07/07 22:55:55, stevenjb wrote: > This could all be simplified. Also, I believe that session_id predates > window_key and should only be used for panels if window_key is not set, e.g: > > std::string shelf_id = app_window->extension_id(); > if (app_window->show_in_shelf() || app_window->window_type_is_panel()) { > if (!app_window->window_key().empty()) > shelf_id += app_window->window_key(); > else > shelf_id += base::StringToInt(app_window->session_id().id()); > } > return shelf_id; > > Note: This would also make it OK for window_key() to be empty since session_id > will always be unique per window (I had forgotten about it). We shouldn't allow > items with an empty window_key/id to be pinned, but they could still have their > own icon within a session without specifying 'id' in the create params. Thank you Steven for the feedback, we will include the simpler version of this in the new patch set. Yes, the unique id will still be needed for pinning purposes, but we could have windows opened with showInShelf=true without pinning them (now, for simplicity, as you said in the previous comments, we have enforced it to always have an 'id' set, either the window will be pinned or not).
https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:37: app_window->extension_id(); On 2016/07/08 07:20:27, andra.paraschiv wrote: > On 2016/07/07 22:55:55, stevenjb wrote: > > This could all be simplified. Also, I believe that session_id predates > > window_key and should only be used for panels if window_key is not set, e.g: > > > > std::string shelf_id = app_window->extension_id(); > > if (app_window->show_in_shelf() || app_window->window_type_is_panel()) { > > if (!app_window->window_key().empty()) > > shelf_id += app_window->window_key(); > > else > > shelf_id += base::StringToInt(app_window->session_id().id()); > > } > > return shelf_id; > > > > Note: This would also make it OK for window_key() to be empty since session_id > > will always be unique per window (I had forgotten about it). We shouldn't > allow > > items with an empty window_key/id to be pinned, but they could still have > their > > own icon within a session without specifying 'id' in the create params. > > Thank you Steven for the feedback, we will include the simpler version of this > in the new patch set. Yes, the unique id will still be needed for pinning > purposes, but we could have windows opened with showInShelf=true without pinning > them (now, for simplicity, as you said in the previous comments, we have > enforced it to always have an 'id' set, either the window will be pinned or > not). Done.
lgtm
lgtm https://codereview.chromium.org/1914993002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:29: // parameter is true the window key value is appended to the app_shelf_id. Update comment.
https://codereview.chromium.org/1914993002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:29: // parameter is true the window key value is appended to the app_shelf_id. On 2016/07/11 19:45:13, stevenjb wrote: > Update comment. Done.
The CQ bit was checked by valentin.ilie@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1914993002/#ps180001 (title: "Rebase + Fixes v7")
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
The author valentin.ilie@intel.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by valentin.ilie@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by valentin.ilie@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1914993002/#ps200001 (title: "Rebase + Fixes v8")
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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/07/13 10:54:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hello Steven, Reilly, Tests LauncherPlatformAppBrowserTest{LaunchPinned , UnpinRunning} from chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc were failing. We did a patch that addressed that case (Fix LaunchPinned/UnpinRunning v9). Could you look into it to see if it looks good to you? Thank you, Valentin
Hooray for tests. https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:163: } else if (app_shelf_id == app_id) { Add a comment: // show_in_shelf is false and not a panel. https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:178: } We should be able to do this with find + lambda, e.g. something like: const auto& id_map = app_shelf_id_to_shelf_id_map_; if (std::find_if(id_map.begin(), id_map.end(), [shelf_id](iter) { return iter->second == shelf_id; }) != id_map.end()) { shelf_id = 0; }
https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:163: } else if (app_shelf_id == app_id) { On 2016/07/15 16:58:31, stevenjb wrote: > Add a comment: > // show_in_shelf is false and not a panel. Done. https://codereview.chromium.org/1914993002/diff/220001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:178: } On 2016/07/15 16:58:31, stevenjb wrote: > We should be able to do this with find + lambda, e.g. something like: > > const auto& id_map = app_shelf_id_to_shelf_id_map_; > if (std::find_if(id_map.begin(), id_map.end(), [shelf_id](iter) { > return iter->second == shelf_id; }) != id_map.end()) { > shelf_id = 0; > } Done.
The CQ bit was checked by valentin.ilie@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1914993002/#ps240001 (title: "Rebase + Fixes v10")
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 better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ========== to ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. ========== to ========== Enhance chrome.app.window API with better shelf integration Enhance chrome.app.window API with the possibility of creating a window that will show up separately in the shelf, tied to its own icon. Added a new property, showInShelf, to CreateWindowOptions. Default value for the property is false. Based on https://codereview.chromium.org/1811523002 Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com> Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com> BUG=610299 TEST=interactive_ui_tests Implement a simple extension that creates windows with the property set to true. Observe that the newly created windows are added as separate icons in the shelf. Committed: https://crrev.com/3049b971f57ee97b1a602605e90ca18223a30653 Cr-Commit-Position: refs/heads/master@{#405978} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3049b971f57ee97b1a602605e90ca18223a30653 Cr-Commit-Position: refs/heads/master@{#405978} |
