Enhance chrome.app.window API for shelf integration with pinning support

Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST= https://codereview.chromium.org/2297633002

Committed: https://crrev.com/49ef88e5d3371340c6b04123b3659207995dbe09
Cr-Commit-Position: refs/heads/master@{#416252}

[Andra Paraschiv, 2016-08-29 11:24:05]

Description was changed from

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
==========

to

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
==========

[Andra Paraschiv, 2016-08-29 11:24:05]

andra.paraschiv@intel.com changed reviewers:
+ oshima@chromium.org, reillyg@chromium.org, skuhne@chromium.org,
stevenjb@chromium.org

[Andra Paraschiv, 2016-08-29 11:37:42]

Uploaded a new CL for showInShelf=true window pinning support.

Began working on including in the codebase the extra launch data needed for
pinning/restore e.g. app_shelf_id in order to support multiple entries per
app_id.
 
Thank you.

[stevenjb, 2016-08-29 15:56:30]

This approach introduces a lot of small confusing changes. We should either:
a) Create a second delegate method that takes a shelf id.
b) Pass "" as the shelf id instead of repeating the app id.

I think a) is probably better since we would need to change less code.

Also, I think that calling the extra parameter "shelf id" is confusing, since we
are actually a) appending it to the app id to create a unique shelf id, and b)
passing it to the app as a launch parameter. So maybe 'launch id' would be a
better name.

Then the new delegate method could be something like
'GetShelfIDForAppIDAndLaunchID' (instead og GetShelfIDForAppIDandShelfID which
is confusing).

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_dele...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_dele...
ash/common/shelf/shelf_delegate.h:39: // Get the shelf ID from an application
ID.
The comment needs to be updated, but this is kind of confusing. I think we may
need a separate delegate method.

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:700: drag_and_drop_app_id_,
drag_and_drop_app_id_);
If we use a single method, we should use "" for the "shelf id" rather than
repeating the app id.

https://codereview.chromium.org/2290603002/diff/1/ash/test/test_shelf_delegat...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/1/ash/test/test_shelf_delegat...
ash/test/test_shelf_delegate.cc:101: const std::string& app_shelf_id) {
To document that app_shelf_id is unused:

cons std::string& /* app_shelf_id */

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:86:
app_shelf_id_(app_shelf_id) {
These appear to be unused?

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:111:
const std::string app_shelf_id_;
Document these

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:909: i !=
id_to_item_controller_map_.end(); ++i) {
Convert this to a C++11 style iterator

[Andra Paraschiv, 2016-08-30 09:58:24]

Thank you, Steven, for feedback.

We can move forward with this approach. I updated the CL and included a
separated delegate method for the extra launch id.

Next, we should further add support for launch id in the remaining codebase e.g.
prefs store, launcher/restore event and also include additional unit tests for
this.

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_dele...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_dele...
ash/common/shelf/shelf_delegate.h:39: // Get the shelf ID from an application
ID.
On 2016/08/29 15:56:30, stevenjb wrote:
> The comment needs to be updated, but this is kind of confusing. I think we may
> need a separate delegate method.

Added a separate delegate method that includes the launch id.

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2290603002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:700: drag_and_drop_app_id_,
drag_and_drop_app_id_);
On 2016/08/29 15:56:30, stevenjb wrote:
> If we use a single method, we should use "" for the "shelf id" rather than
> repeating the app id.

Done.

https://codereview.chromium.org/2290603002/diff/1/ash/test/test_shelf_delegat...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/1/ash/test/test_shelf_delegat...
ash/test/test_shelf_delegate.cc:101: const std::string& app_shelf_id) {
On 2016/08/29 15:56:30, stevenjb wrote:
> To document that app_shelf_id is unused:
> 
> cons std::string& /* app_shelf_id */

Done.

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:86:
app_shelf_id_(app_shelf_id) {
On 2016/08/29 15:56:30, stevenjb wrote:
> These appear to be unused?

They are currently used in the GetShelfIDForAppIDAndLaunchID method to check for
app shortcut app_id and launch_id values.

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:111:
const std::string app_shelf_id_;
On 2016/08/29 15:56:30, stevenjb wrote:
> Document these

Done.

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:909: i !=
id_to_item_controller_map_.end(); ++i) {
On 2016/08/29 15:56:30, stevenjb wrote:
> Convert this to a C++11 style iterator

Done.

[stevenjb, 2016-08-30 16:12:52]

Definitely more clear now, thanks!

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:86:
app_shelf_id_(app_shelf_id) {
On 2016/08/30 09:58:24, andra.paraschiv wrote:
> On 2016/08/29 15:56:30, stevenjb wrote:
> > These appear to be unused?
> 
> They are currently used in the GetShelfIDForAppIDAndLaunchID method to check
for
> app shortcut app_id and launch_id values.

Right, sorry, I was confused by the multiple meanings of app_shelf_id. launch_id
is much more clear, thanks for changing it.

https://codereview.chromium.org/2290603002/diff/40001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/40001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:115: return 0;
Instead of copy/pasting identical code, just return GetShelfIDForAppID(app_id).

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:79: const
std::string& app_id,
Yikes. Please change this to app_id, launch_id so that the order is consistent
with everything else.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:357:
std::string launch_id = "";
' = ""' unnecessary.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:906:
ash::ShelfID shelf_id = GetShelfIDForAppIDAndLaunchID(app_id, "");
No need for intermediate |shelf_id|

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:917: // If app
window controller, check app_id and launch_id.
Move these comments inside the if clauses

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:938: }
These tests are the same, why cast them? e.g.:

if (type == APP || (type == SHORTCUT && app_id != kChrome) {
  if (second->app_id() == app_id && second->launc_id() == launc_id)
    return first;
} else ...

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:46:
std::string app_shelf_id = app_id + launch_id;
Nice this is much more clear now, thanks!
nit: return app_id + launch_id;

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:139:
std::string launch_id = GetLaunchId(app_window);
Move this to where it is used.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:168:
// show_in_shelf is false and not a panel
This comment looks like it is associated with the if clause above, please move
it inside the else clause.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:171:
ash::WmShell::Get()->shelf_delegate()->GetShelfIDForAppID(app_id);
These can be combined now, right? launch_id should be empty iff the if in line
164 is false.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:26:
const std::string& app_id,
Switch order

[Andra Paraschiv, 2016-08-31 10:57:19]

Thank you for review.

https://codereview.chromium.org/2290603002/diff/40001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/40001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:115: return 0;
On 2016/08/30 16:12:51, stevenjb wrote:
> Instead of copy/pasting identical code, just return
GetShelfIDForAppID(app_id).
>  

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:79: const
std::string& app_id,
On 2016/08/30 16:12:51, stevenjb wrote:
> Yikes. Please change this to app_id, launch_id so that the order is consistent
> with everything else.

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:357:
std::string launch_id = "";
On 2016/08/30 16:12:51, stevenjb wrote:
> ' = ""' unnecessary.

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:906:
ash::ShelfID shelf_id = GetShelfIDForAppIDAndLaunchID(app_id, "");
On 2016/08/30 16:12:52, stevenjb wrote:
> No need for intermediate |shelf_id|

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:917: // If app
window controller, check app_id and launch_id.
On 2016/08/30 16:12:52, stevenjb wrote:
> Move these comments inside the if clauses

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:938: }
On 2016/08/30 16:12:51, stevenjb wrote:
> These tests are the same, why cast them? e.g.:
> 
> if (type == APP || (type == SHORTCUT && app_id != kChrome) {
>   if (second->app_id() == app_id && second->launc_id() == launc_id)
>     return first;
> } else ...
> 

Yes, they are the same, but we should include the launch_id in the
LauncherItemController general class to do this checks without specific casts. 

Updated this class to include the launch_id field.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:46:
std::string app_shelf_id = app_id + launch_id;
On 2016/08/30 16:12:52, stevenjb wrote:
> Nice this is much more clear now, thanks!
> nit: return app_id + launch_id;

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:139:
std::string launch_id = GetLaunchId(app_window);
On 2016/08/30 16:12:52, stevenjb wrote:
> Move this to where it is used.

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:168:
// show_in_shelf is false and not a panel
On 2016/08/30 16:12:52, stevenjb wrote:
> This comment looks like it is associated with the if clause above, please move
> it inside the else clause.

Done.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:171:
ash::WmShell::Get()->shelf_delegate()->GetShelfIDForAppID(app_id);
On 2016/08/30 16:12:52, stevenjb wrote:
> These can be combined now, right? launch_id should be empty iff the if in line
> 164 is false.

Yes, we can combine these now.

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:26:
const std::string& app_id,
On 2016/08/30 16:12:52, stevenjb wrote:
> Switch order

Done.

[Reilly Grant (use Gerrit), 2016-08-31 17:57:41]

Can you better document what a "launch ID" is? How is it different from a shelf
ID? It seems like it should be a "launcher ID" since I think it corresponds to a
LauncherItem.

[stevenjb, 2016-08-31 18:11:43]

On 2016/08/31 17:57:41, Reilly Grant wrote:
> Can you better document what a "launch ID" is? How is it different from a
shelf
> ID? It seems like it should be a "launcher ID" since I think it corresponds to
a
> LauncherItem.

The intention is that an app can provide an identifier that is sent to the app
when a pinned window is clicked on ("launched").

In order to provide a unique shelf id for every icon in the shelf, we combine
the app id and the launch id to create the shelf id.

Currently we use app_window->window_key() for the launch id if provided,
otherwise we fall back to the window's session id.

We should stay away from the word "launcher"; that *used to* be used to describe
the shelf, but now only refers to the App Launcher feature (app_list in the
code). We really ought to rename the 'launcher' source files...

[stevenjb, 2016-08-31 18:24:28]

lgtm with clarified comments.

Reilly, if 'launch_id' is confusing, we could use 'window_key' instead to be
consistent with AppWindow. The trouble with that is that it may not be
window_key if that is not provided (e.g. if an app wants to create a separate
shelf item for the session only).

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:938: }
On 2016/08/31 10:57:18, andra.paraschiv wrote:
> On 2016/08/30 16:12:51, stevenjb wrote:
> > These tests are the same, why cast them? e.g.:
> > 
> > if (type == APP || (type == SHORTCUT && app_id != kChrome) {
> >   if (second->app_id() == app_id && second->launc_id() == launc_id)
> >     return first;
> > } else ...
> > 
> 
> Yes, they are the same, but we should include the launch_id in the
> LauncherItemController general class to do this checks without specific casts.

> 
> Updated this class to include the launch_id field.

Ahh, I didn't realize that launch_id() didn't exist in LauncherItemController.
We could make GetLaunchId() a viertual function instead, but this way seems
fine.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_item_controller.h (right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_item_controller.h:95: // The launch id
associated with the window.
This doesn't explain what a 'launch id' is. This should be something like:
// An id that can be passed to an app when launched in order to support multiple
shelf (launcher) items per app.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_item_controller.h:97: ash::ShelfID
shelf_id_;
Add a comment for shelf_id to help reduce the naming confusion, e.g.:
// A unique id for the shelf item (based on app_id and launch_id).

[Reilly Grant (use Gerrit), 2016-08-31 19:14:28]

lgtm, thanks for the clarification.

[Mr4D (OOO till 08-26), 2016-08-31 21:52:53]

lgtm

[oshima, 2016-08-31 22:59:12]

oshima@chromium.org changed reviewers:
+ jamescook@chromium.org

[oshima, 2016-08-31 22:59:12]

-> jamescook@

[James Cook, 2016-08-31 23:33:16]

On 2016/08/31 22:59:12, oshima wrote:
> -> jamescook@

Is there a design doc or extension API review document for this new parameter?

[James Cook, 2016-09-01 04:27:52]

Please write a test for the new pinning behavior, or cite the specific test that
covers it in the CL description.

(Bugs in this feature are not likely to be discovered by casual users, so I want
to be sure we have automated tests.)

https://codereview.chromium.org/2290603002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:48: const std::string& launch_id) = 0;
|launch_id| needs to be documented at this level as well.

(Steven, are you sure you like launch_id? I find it pretty confusing.
app_window_id or app_window_key or window_key might be better. We just use the
word "launch" too much.)

https://codereview.chromium.org/2290603002/diff/60001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/60001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:110: const std::string& /* launch_id */) {
nit: you don't need the /* */ -- we have unused parameters everywhere in the
code base

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:112: //
The launch id associated with this app shortcut.
This needs to document what |launch_id_| *means*, or refer to a canonical
comment somewhere else that explains it.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:911: if
((id_to_item_controller_pair.second->app_id() == app_id) &&
nit: no need for extra () around == condition

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:32:
std::string launch_id = "";
nit: no need for = "";

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:35:
launch_id += app_window->window_key();
don't use += since the string starts empty

[James Cook, 2016-09-01 04:27:54]

Please write a test for the new pinning behavior, or cite the specific test that
covers it in the CL description.

(Bugs in this feature are not likely to be discovered by casual users, so I want
to be sure we have automated tests.)

[James Cook, 2016-09-01 04:27:56]

Please write a test for the new pinning behavior, or cite the specific test that
covers it in the CL description.

(Bugs in this feature are not likely to be discovered by casual users, so I want
to be sure we have automated tests.)

[Andra Paraschiv, 2016-09-01 08:27:25]

Thank you all for feedback and Steven for clarifications.

Yes, there are several ids used for each shelf item:

* app_id (string) - the application id (currently it is the extension id).

* launch_id (string) - the id that can be passed to an app when it is launched
in order to support multiple items shown in shelf per app_id.

* app_shelf_id (string) - a unique id per shelf item - app_shelf_id = app_id +
launch_id.

This is used to identify each shelf item and check if there is already a
shelf_id assigned to it by the shelf model e.g. the item is pinned. In case
there is no shelf_id for the item (shelf_id = 0), a new shelf item is created
and a shelf_id is assigned to it.  

* shelf_id (int) - a unique id assigned by the shelf model for each shelf item.

The design doc is available here -
https://bugs.chromium.org/p/chromium/issues/detail?id=610299.

We have another CL - https://codereview.chromium.org/2297633002/ - where we
should add unit tests for items shown in shelf.

After getting this patch set done and deciding on the launch_id code name,
should we land this as is and create new CLs to add further support for
launch_id in e.g. prefs and launch event code base in order to have full pinning
support for multiple shelf items per app_id? Or should we continue to add new
patch sets to this CL?

I'm asking this because the full pinning/restore support covers more code base
than the previous requirements for Launcher and Shelf feature.

https://codereview.chromium.org/2290603002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:48: const std::string& launch_id) = 0;
On 2016/09/01 04:27:51, James Cook wrote:
> |launch_id| needs to be documented at this level as well.
> 
> (Steven, are you sure you like launch_id? I find it pretty confusing.
> app_window_id or app_window_key or window_key might be better. We just use the
> word "launch" too much.)

Added the comment for launch_id.

https://codereview.chromium.org/2290603002/diff/60001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2290603002/diff/60001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:110: const std::string& /* launch_id */) {
On 2016/09/01 04:27:51, James Cook wrote:
> nit: you don't need the /* */ -- we have unused parameters everywhere in the
> code base

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:112: //
The launch id associated with this app shortcut.
On 2016/09/01 04:27:51, James Cook wrote:
> This needs to document what |launch_id_| *means*, or refer to a canonical
> comment somewhere else that explains it.

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:911: if
((id_to_item_controller_pair.second->app_id() == app_id) &&
On 2016/09/01 04:27:52, James Cook wrote:
> nit: no need for extra () around == condition

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:32:
std::string launch_id = "";
On 2016/09/01 04:27:52, James Cook wrote:
> nit: no need for = "";

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:35:
launch_id += app_window->window_key();
On 2016/09/01 04:27:52, James Cook wrote:
> don't use += since the string starts empty

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_item_controller.h (right):

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_item_controller.h:95: // The launch id
associated with the window.
On 2016/08/31 18:24:28, stevenjb wrote:
> This doesn't explain what a 'launch id' is. This should be something like:
> // An id that can be passed to an app when launched in order to support
multiple
> shelf (launcher) items per app.

Done.

https://codereview.chromium.org/2290603002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_item_controller.h:97: ash::ShelfID
shelf_id_;
On 2016/08/31 18:24:27, stevenjb wrote:
> Add a comment for shelf_id to help reduce the naming confusion, e.g.:
> // A unique id for the shelf item (based on app_id and launch_id).

Done.

[James Cook, 2016-09-01 16:15:18]

LGTM assuming we settle on a name. I'll defer to Steven on that one -- stick
with launch_id? Use app_window_key or app_launch_id or app_launch_param?

https://codereview.chromium.org/2290603002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:48: // uniquely identify each shelf item that
has the same app_id.
nit: I would also say something like: "For example, a single virtualization app
might want to show different shelf icons for different remote apps."  People
reason well from examples.

[James Cook, 2016-09-01 23:01:28]

I chatted with Steven. |launch_id| is fine. Please make sure the tests you add
in the next CL cover the pinning behavior.

[Andra Paraschiv, 2016-09-02 07:24:27]

Description was changed from

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
==========

to

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST=https://codereview.chromium.org/2297633002
==========

[Andra Paraschiv, 2016-09-02 07:30:17]

Description was changed from

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST=https://codereview.chromium.org/2297633002
==========

to

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST= https://codereview.chromium.org/2297633002
==========

[Andra Paraschiv, 2016-09-02 07:35:44]

Ok, James, thank you for review.

I updated the CL description to include the link to the unit tests CL.

https://codereview.chromium.org/2290603002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_delegate.h (right):

https://codereview.chromium.org/2290603002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:48: // uniquely identify each shelf item that
has the same app_id.
On 2016/09/01 16:15:18, James Cook wrote:
> nit: I would also say something like: "For example, a single virtualization
app
> might want to show different shelf icons for different remote apps."  People
> reason well from examples.

Done.

[Andra Paraschiv, 2016-09-02 08:22:10]

The CQ bit was checked by andra.paraschiv@intel.com

[Andra Paraschiv, 2016-09-02 08:22:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, stevenjb@chromium.org, skuhne@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2290603002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-02 08:22:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 08:34:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 08:34:08]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Andra Paraschiv, 2016-09-02 11:09:32]

The CQ bit was checked by andra.paraschiv@intel.com

[Andra Paraschiv, 2016-09-02 11:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, stevenjb@chromium.org, skuhne@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2290603002/#ps140001
(title: "Shelf Delegate Fix after Rebase")

[commit-bot: I haz the power, 2016-09-02 11:09:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 11:25:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 11:25:39]

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_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Andra Paraschiv, 2016-09-02 11:56:40]

The CQ bit was checked by andra.paraschiv@intel.com

[Andra Paraschiv, 2016-09-02 11:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, stevenjb@chromium.org, skuhne@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2290603002/#ps160001
(title: "Shelf Delegate Unit Tests Fix after Rebase")

[commit-bot: I haz the power, 2016-09-02 11:56:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Andra Paraschiv, 2016-09-02 12:09:02]

The CQ bit was unchecked by andra.paraschiv@intel.com

[Andra Paraschiv, 2016-09-02 12:19:55]

The CQ bit was checked by andra.paraschiv@intel.com

[Andra Paraschiv, 2016-09-02 12:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, stevenjb@chromium.org, skuhne@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2290603002/#ps180001
(title: "LauncherItemController Unit Tests Fix after Rebase")

[commit-bot: I haz the power, 2016-09-02 12:20:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 13:10:19]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-02 13:12:12]

Description was changed from

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST= https://codereview.chromium.org/2297633002
==========

to

==========
Enhance chrome.app.window API for shelf integration with pinning support

Enhance the chrome.app.window API for shelf integration with the possibility of
pinning the showInShelf=true windows.

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 showInShelf property set
to true. Pin a newly created window that has its own icon in the shelf, then
close it and open it again by clicking the pinned icon.
TEST= https://codereview.chromium.org/2297633002

Committed: https://crrev.com/49ef88e5d3371340c6b04123b3659207995dbe09
Cr-Commit-Position: refs/heads/master@{#416252}
==========

[commit-bot: I haz the power, 2016-09-02 13:12:13]

Patchset 10 (id:??) landed as
https://crrev.com/49ef88e5d3371340c6b04123b3659207995dbe09
Cr-Commit-Position: refs/heads/master@{#416252}