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

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

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

Based on https://codereview.chromium.org/2290603002
Based on https://codereview.chromium.org/2352353002

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/4b52d653d510dc49d944b824d0aa7d6ef41b8924
Cr-Commit-Position: refs/heads/master@{#427323}

[Andra Paraschiv, 2016-09-19 09:28:36]

Description was changed from

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

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

Based on https://codereview.chromium.org/2290603002

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 (cont.)

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

Based on https://codereview.chromium.org/2290603002

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-19 09:28:36]

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

[stevenjb, 2016-09-20 16:43:42]

On 2016/09/19 09:28:36, Andra Paraschiv wrote:
> mailto:andra.paraschiv@intel.com changed reviewers:
> + mailto:jamescook@chromium.org, mailto:reillyg@chromium.org,
mailto:skuhne@chromium.org,
> mailto:stevenjb@chromium.org

We shouldn't rely on app_id + launch_id throughout the code for a unique chrome
launcher identifier, it is much too fragile.

Instead, we should replace app_id in chrome_launcher_prefs with
app_launcher_id*.

* "launch id" = extra parameter passed to the app when it is launched
* "launcher id" = unique identifier for items in the launcher

Regardless, we need a single class or method that creates unique ids for shelf
items. Maybe we can just use ash::ShelfID?

This is going to be a bit tricky and subtle to get right, I apologize for not
having time to look more closely at the code and give more specific guidance.

[James Cook, 2016-09-20 17:20:54]

On 2016/09/20 16:43:42, stevenjb wrote:
> On 2016/09/19 09:28:36, Andra Paraschiv wrote:
> > mailto:andra.paraschiv@intel.com changed reviewers:
> > + mailto:jamescook@chromium.org, mailto:reillyg@chromium.org,
> mailto:skuhne@chromium.org,
> > mailto:stevenjb@chromium.org
> 
> We shouldn't rely on app_id + launch_id throughout the code for a unique
chrome
> launcher identifier, it is much too fragile.
> 
> Instead, we should replace app_id in chrome_launcher_prefs with
> app_launcher_id*.
> 
> * "launch id" = extra parameter passed to the app when it is launched
> * "launcher id" = unique identifier for items in the launcher
> 
> Regardless, we need a single class or method that creates unique ids for shelf
> items. Maybe we can just use ash::ShelfID?
> 
> This is going to be a bit tricky and subtle to get right, I apologize for not
> having time to look more closely at the code and give more specific guidance.

ash::ShelfID is just an int, and I don't think it's necessarily stable between
runs.

https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_model.h?sq=packag...

Steven, what about a tiny class or struct called something like AppShelfId or
AppLauncherId that consists of two strings (app_id and launch_id)? I think it
could have default constructor, comparison operators, etc. It would be a bit of
a pain to find/replace all the app_id usage, but it would be a mechanical
change, then things would be type-safe.

[stevenjb, 2016-09-20 18:14:30]

Ah, good point.

Yeah, I think an AppLauncherId wrapper class would make sense. We could
split this into two CLs, the first would just introduce AppLauncherId with
a single string constructor, then we could add a second constructor that
also takes a launch id in a follow-up.



On Tue, Sep 20, 2016 at 11:20 AM, <jamescook@chromium.org> wrote:

> On 2016/09/20 16:43:42, stevenjb wrote:
> > On 2016/09/19 09:28:36, Andra Paraschiv wrote:
> > > mailto:andra.paraschiv@intel.com changed reviewers:
> > > + mailto:jamescook@chromium.org, mailto:reillyg@chromium.org,
> > mailto:skuhne@chromium.org,
> > > mailto:stevenjb@chromium.org
> >
> > We shouldn't rely on app_id + launch_id throughout the code for a unique
> chrome
> > launcher identifier, it is much too fragile.
> >
> > Instead, we should replace app_id in chrome_launcher_prefs with
> > app_launcher_id*.
> >
> > * "launch id" = extra parameter passed to the app when it is launched
> > * "launcher id" = unique identifier for items in the launcher
> >
> > Regardless, we need a single class or method that creates unique ids for
> shelf
> > items. Maybe we can just use ash::ShelfID?
> >
> > This is going to be a bit tricky and subtle to get right, I apologize
> for not
> > having time to look more closely at the code and give more specific
> guidance.
>
> ash::ShelfID is just an int, and I don't think it's necessarily stable
> between
> runs.
>
> https://cs.chromium.org/chromium/src/ash/common/shelf/
> shelf_model.h?sq=package:chromium&dr=C&rcl=1474370620&l=108
>
> Steven, what about a tiny class or struct called something like AppShelfId
> or
> AppLauncherId that consists of two strings (app_id and launch_id)? I think
> it
> could have default constructor, comparison operators, etc. It would be a
> bit of
> a pain to find/replace all the app_id usage, but it would be a mechanical
> change, then things would be type-safe.
>
>
> https://codereview.chromium.org/2341643002/
>

-- 
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.

[Andra Paraschiv, 2016-09-21 10:25:26]

Description was changed from

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

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

Based on https://codereview.chromium.org/2290603002

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 (cont.)

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

Based on https://codereview.chromium.org/2290603002
Based on https://codereview.chromium.org/2352353002

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-21 10:30:22]

On 2016/09/20 18:14:30, stevenjb wrote:
> Ah, good point.
> 
> Yeah, I think an AppLauncherId wrapper class would make sense. We could
> split this into two CLs, the first would just introduce AppLauncherId with
> a single string constructor, then we could add a second constructor that
> also takes a launch id in a follow-up.
> 
> 
> 
> On Tue, Sep 20, 2016 at 11:20 AM, <mailto:jamescook@chromium.org> wrote:
> 
> > On 2016/09/20 16:43:42, stevenjb wrote:
> > > On 2016/09/19 09:28:36, Andra Paraschiv wrote:
> > > > mailto:andra.paraschiv@intel.com changed reviewers:
> > > > + mailto:jamescook@chromium.org, mailto:reillyg@chromium.org,
> > > mailto:skuhne@chromium.org,
> > > > mailto:stevenjb@chromium.org
> > >
> > > We shouldn't rely on app_id + launch_id throughout the code for a unique
> > chrome
> > > launcher identifier, it is much too fragile.
> > >
> > > Instead, we should replace app_id in chrome_launcher_prefs with
> > > app_launcher_id*.
> > >
> > > * "launch id" = extra parameter passed to the app when it is launched
> > > * "launcher id" = unique identifier for items in the launcher
> > >
> > > Regardless, we need a single class or method that creates unique ids for
> > shelf
> > > items. Maybe we can just use ash::ShelfID?
> > >
> > > This is going to be a bit tricky and subtle to get right, I apologize
> > for not
> > > having time to look more closely at the code and give more specific
> > guidance.
> >
> > ash::ShelfID is just an int, and I don't think it's necessarily stable
> > between
> > runs.
> >
> > https://cs.chromium.org/chromium/src/ash/common/shelf/
> > shelf_model.h?sq=package:chromium&dr=C&rcl=1474370620&l=108
> >
> > Steven, what about a tiny class or struct called something like AppShelfId
> > or
> > AppLauncherId that consists of two strings (app_id and launch_id)? I think
> > it
> > could have default constructor, comparison operators, etc. It would be a
> > bit of
> > a pain to find/replace all the app_id usage, but it would be a mechanical
> > change, then things would be type-safe.
> >
> >
> > https://codereview.chromium.org/2341643002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Thank you both for feedback, we can move forward with this wrapper approach.

I created a new CL - https://codereview.chromium.org/2352353002 - to include in
the prefs codebase the AppLauncherId class with one string (app_id) constructor.
After this CL is ready to be landed, I'll come back to the current CL, include
the app_id + launch_id constructor and move forward with adding pinning support.

[Andra Paraschiv, 2016-10-20 09:01:43]

> Thank you both for feedback, we can move forward with this wrapper approach.
> 
> I created a new CL - https://codereview.chromium.org/2352353002 - to include
in
> the prefs codebase the AppLauncherId class with one string (app_id)
constructor.
> After this CL is ready to be landed, I'll come back to the current CL, include
> the app_id + launch_id constructor and move forward with adding pinning
support.

I updated the CL to include the AppLauncherId wrapper changes.

[James Cook, 2016-10-21 15:11:04]

I only looked at //ash

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

https://codereview.chromium.org/2341643002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:44: virtual const std::string&
GetLaunchIDForShelfID(ShelfID id) = 0;
I don't see any calls to this inside //ash. If it is only used in //chrome, it
doesn't need to be a method on the ash delegate -- the code in chrome should
look it up in that layer.

(//ash is conceptually "below" //chrome in the dependency hierarchy. These
delegates exist so that ash can occasionally call "up" to get a piece of
information it needs.)

[stevenjb, 2016-10-21 21:40:40]

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

https://codereview.chromium.org/2341643002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:150: const
std::string& GetLaunchIDForShelfID(ash::ShelfID id) override;
I agree with James' earlier comment. We can make this part of
ChromeLauncherController and eliminate a lot of unnecessary changes.

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

https://codereview.chromium.org/2341643002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:2377:
}
We should have at least one test that pins an app with multiple launch ids if we
don't already. That can be added separately.

[Reilly Grant (use Gerrit), 2016-10-21 21:42:08]

reillyg@chromium.org changed reviewers:
- reillyg@chromium.org

[Andra Paraschiv, 2016-10-24 08:26:16]

Thank you both for review.

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

https://codereview.chromium.org/2341643002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_delegate.h:44: virtual const std::string&
GetLaunchIDForShelfID(ShelfID id) = 0;
On 2016/10/21 15:11:03, James Cook wrote:
> I don't see any calls to this inside //ash. If it is only used in //chrome, it
> doesn't need to be a method on the ash delegate -- the code in chrome should
> look it up in that layer.
> 
> (//ash is conceptually "below" //chrome in the dependency hierarchy. These
> delegates exist so that ash can occasionally call "up" to get a piece of
> information it needs.)

Thank you, James, for the explanation, I moved this in the //chrome layer and
removed unnecessary changes from //ash.

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

https://codereview.chromium.org/2341643002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:150: const
std::string& GetLaunchIDForShelfID(ash::ShelfID id) override;
On 2016/10/21 21:40:40, stevenjb wrote:
> I agree with James' earlier comment. We can make this part of
> ChromeLauncherController and eliminate a lot of unnecessary changes.

Thank you for this, Steven, I moved it from ash delegate to chrome launcher
controller.

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

https://codereview.chromium.org/2341643002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:2377:
}
On 2016/10/21 21:40:40, stevenjb wrote:
> We should have at least one test that pins an app with multiple launch ids if
we
> don't already. That can be added separately.

Sure, these tests for multiple launch ids should be added to this CL -
https://codereview.chromium.org/2297633002/. I already added tests for pinning
support for one showInShelf=true window.

[Mr4D (OOO till 08-26), 2016-10-24 14:25:08]

I would like to see a test for this.

Furthermore: I thought that pinning was something which was forced by the
administrator to create "kiosk" machines, school tests, etc.).
It feels odd that a user would start such an app he "cannot escape from" to the
shelf. Is that really what we want?

[stevenjb, 2016-10-24 16:45:07]

On 2016/10/24 14:25:08, Mr4D wrote:
> I would like to see a test for this.
> 
> Furthermore: I thought that pinning was something which was forced by the
> administrator to create "kiosk" machines, school tests, etc.).
> It feels odd that a user would start such an app he "cannot escape from" to
the
> shelf. Is that really what we want?

skuhne@ - I think you may be confusing this with a different but unrelated
effort for fullscreen pinning?

This feature is intended to allow apps like Citrix to have multiple app icons
pinned to the launcher. Each icon would send a launch message to the app with a
unique "launch id", e.g. "Word" or "Excel".

[stevenjb, 2016-10-24 16:46:58]

lgtm

[Mr4D (OOO till 08-26), 2016-10-24 16:52:46]

In deed. I see - thanks for the clarification! 

lgtm.

Please provide unit test either as follow up or include in this CL.

[James Cook, 2016-10-24 17:52:07]

-me as reviewer since this no longer touches //ash

[Andra Paraschiv, 2016-10-25 07:57:11]

andra.paraschiv@intel.com changed reviewers:
- jamescook@chromium.org

[Andra Paraschiv, 2016-10-25 07:57:12]

Thank you all for feedback, I added new unit tests for pinning in this CL -
https://codereview.chromium.org/2297633002/.

[Andra Paraschiv, 2016-10-25 10:42:55]

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

[commit-bot: I haz the power, 2016-10-25 10:43:05]

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-10-25 10:53:50]

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

[commit-bot: I haz the power, 2016-10-25 10:53:51]

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

[Andra Paraschiv, 2016-10-25 11:11:04]

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

[Andra Paraschiv, 2016-10-25 11:11:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2341643002/#ps100001
(title: "Unit Test Fix")

[commit-bot: I haz the power, 2016-10-25 11:11:16]

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-10-25 11:36:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-25 11:38:01]

Description was changed from

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

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

Based on https://codereview.chromium.org/2290603002
Based on https://codereview.chromium.org/2352353002

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 (cont.)

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

Based on https://codereview.chromium.org/2290603002
Based on https://codereview.chromium.org/2352353002

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/4b52d653d510dc49d944b824d0aa7d6ef41b8924
Cr-Commit-Position: refs/heads/master@{#427323}
==========

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

Patchset 6 (id:??) landed as
https://crrev.com/4b52d653d510dc49d944b824d0aa7d6ef41b8924
Cr-Commit-Position: refs/heads/master@{#427323}