mash: Support ShelfWindowWatcher via ShelfItem properties.

mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org

Committed: https://crrev.com/6ab444005ded90d3b70ad8bd086888307dcc924a
Cr-Commit-Position: refs/heads/master@{#421788}

[msw, 2016-09-27 21:39:00]

Patchset #5 (id:80001) has been deleted

[msw, 2016-09-27 22:40:35]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItemDetails.

BUG=634150
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItemDetails.

Implement placeholder WmWindowMus::GetShelfItemDetails.
(shows shelf items for open windows via ShelfWindowWatcher)

Nix ShelfItemDetails::title, use window titles instead.
(only changes "Settings" tooltip to "Google Chrome - Settings")
Make GetShelfItemDetails return a struct copy.

Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

BUG=634150
==========

[msw, 2016-09-28 15:48:44]

Patchset #7 (id:140001) has been deleted

[msw, 2016-09-28 17:34:53]

Patchset #8 (id:170001) has been deleted

[msw, 2016-09-28 17:38:45]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItemDetails.

Implement placeholder WmWindowMus::GetShelfItemDetails.
(shows shelf items for open windows via ShelfWindowWatcher)

Nix ShelfItemDetails::title, use window titles instead.
(only changes "Settings" tooltip to "Google Chrome - Settings")
Make GetShelfItemDetails return a struct copy.

Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

BUG=634150
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org
==========

[msw, 2016-09-28 17:38:45]

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

[msw, 2016-09-28 17:39:03]

Hey James, please take a look; thanks!

[msw, 2016-09-28 17:39:11]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 17:39:50]

Dry run: 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-28 18:32:15]

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

[commit-bot: I haz the power, 2016-09-28 18:32:16]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-28 20:41:03]

https://codereview.chromium.org/2357143004/diff/190001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:54: DEFINE_WINDOW_PROPERTY_KEY(int, kShelfIconIdKey,
kInvalidImageResourceID);
So on aura, GetWindowProperty(kShelfIconIdKey) for a window with no property set
will return an invalid resource id, whereas on mus it will return a default icon
id? Is that intentional?

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:39:
window->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_DIALOG);
This is fine, but just curious: does it need to change from TYPE_PLATFORM_APP to
TYPE_DIALOG?

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:55: // Adding windows with
valid ShelfItemType properties adds shelf items.
Thanks for fixing all the comments.

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:105:
window2->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_UNDEFINED);
How about adding a test that sets an icon ID but not a type and verifies that
that does nothing (or at least doesn't crash)?

https://codereview.chromium.org/2357143004/diff/190001/ash/mus/bridge/wm_wind...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/mus/bridge/wm_wind...
ash/mus/bridge/wm_window_mus.cc:400: return kInvalidShelfID;
Good catch.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (left):

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:26: #include
"chrome/grit/generated_resources.h"
Nice catch.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:103:
window->AddObserver(this);
Does this observer need to be cleaned up?

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:108: // Override
Chrome's product name prepending for Settings windows.
super nit: Maybe just say "Name the window "Settings" instead of "Google Chrome
- Settings"?

I only know what you're talking about because you asked me about it earlier. :-)

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:109: if
(window->GetTitle() != l10n_util::GetStringUTF16(IDS_SETTINGS_TITLE))
Aside: Ugh, aura::Window::SetTitle calls observers even if the title didn't
change. Bummer.

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:51: // The image resource ID
that should be used as the shelf item's icon.
nit: Type? int?

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:52: const string
kShelfIconId_Property = "prop:shelf-icon-id";
optional: What do you think of kShelfIcon*Resource*Id and shelf-icon-resource-id
(here and elsewhere)? IconId could be confused with ItemId, or something related
to the data model. OTOH, including "resource" makes the identifiers longer.
WDYT?

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:53: // The type of item that
should be shown on the shelf for this window.
ditto

Also, document what happens if this is not provided (and above too). Is it
sufficient to supply only one and get a shelf item?

I'm super picky about documentation in mojoms (and interface classes in
general).

[msw, 2016-09-28 23:26:47]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2016-09-28 23:26:48]

msw@chromium.org changed reviewers:
+ tsepez@chromium.org

[msw, 2016-09-28 23:27:46]

Hey James, please take another look; thanks!
Hey Tom, please take a look at window_manager.mojom, thanks!

https://codereview.chromium.org/2357143004/diff/190001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:54: DEFINE_WINDOW_PROPERTY_KEY(int, kShelfIconIdKey,
kInvalidImageResourceID);
On 2016/09/28 20:41:03, James Cook wrote:
> So on aura, GetWindowProperty(kShelfIconIdKey) for a window with no property
set
> will return an invalid resource id, whereas on mus it will return a default
icon
> id? Is that intentional?

Yes, for now mash relies on ShelfWindowWatcher for *all* windows, that might
change in the very near future, but for now, it seems beneficial.

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:39:
window->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_DIALOG);
On 2016/09/28 20:41:03, James Cook wrote:
> This is fine, but just curious: does it need to change from TYPE_PLATFORM_APP
to
> TYPE_DIALOG?

Nope! Just done to keep it a one-liner. Let me know if I should change it back.

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:55: // Adding windows with
valid ShelfItemType properties adds shelf items.
On 2016/09/28 20:41:03, James Cook wrote:
> Thanks for fixing all the comments.

Acknowledged.

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:105:
window2->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_UNDEFINED);
On 2016/09/28 20:41:03, James Cook wrote:
> How about adding a test that sets an icon ID but not a type and verifies that
> that does nothing (or at least doesn't crash)?

Done.

https://codereview.chromium.org/2357143004/diff/190001/ash/mus/bridge/wm_wind...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/mus/bridge/wm_wind...
ash/mus/bridge/wm_window_mus.cc:400: return kInvalidShelfID;
On 2016/09/28 20:41:03, James Cook wrote:
> Good catch.

Acknowledged.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (left):

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:26: #include
"chrome/grit/generated_resources.h"
On 2016/09/28 20:41:03, James Cook wrote:
> Nice catch.

Acknowledged.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:103:
window->AddObserver(this);
On 2016/09/28 20:41:03, James Cook wrote:
> Does this observer need to be cleaned up?

Done. (used WmWindowTracker instead)

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:108: // Override
Chrome's product name prepending for Settings windows.
On 2016/09/28 20:41:03, James Cook wrote:
> super nit: Maybe just say "Name the window "Settings" instead of "Google
Chrome
> - Settings"?
> 
> I only know what you're talking about because you asked me about it earlier.
:-)

Done.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:109: if
(window->GetTitle() != l10n_util::GetStringUTF16(IDS_SETTINGS_TITLE))
On 2016/09/28 20:41:03, James Cook wrote:
> Aside: Ugh, aura::Window::SetTitle calls observers even if the title didn't
> change. Bummer.

Acknowledged. Worth following up?

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:51: // The image resource ID
that should be used as the shelf item's icon.
On 2016/09/28 20:41:03, James Cook wrote:
> nit: Type? int?

Done.

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:52: const string
kShelfIconId_Property = "prop:shelf-icon-id";
On 2016/09/28 20:41:03, James Cook wrote:
> optional: What do you think of kShelfIcon*Resource*Id and
shelf-icon-resource-id
> (here and elsewhere)? IconId could be confused with ItemId, or something
related
> to the data model. OTOH, including "resource" makes the identifiers longer.
> WDYT?

Done; renamed as you suggest. FYI: I'd like to just remove this and use
kWindowAppIcon_Property, but I'm not sure how that'll work for aura yet...

https://codereview.chromium.org/2357143004/diff/190001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:53: // The type of item that
should be shown on the shelf for this window.
On 2016/09/28 20:41:03, James Cook wrote:
> ditto
> 
> Also, document what happens if this is not provided (and above too). Is it
> sufficient to supply only one and get a shelf item?
> 
> I'm super picky about documentation in mojoms (and interface classes in
> general).

Done.

[msw, 2016-09-28 23:29:30]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 23:29:50]

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

[Tom Sepez, 2016-09-28 23:45:19]

mojom LGTM

[commit-bot: I haz the power, 2016-09-29 00:27:27]

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

[commit-bot: I haz the power, 2016-09-29 00:27:28]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-29 02:49:46]

LGTM with nits, but please look into the icon ID for the multi-profile task
manager case.

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:39:
window->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_DIALOG);
On 2016/09/28 23:27:46, msw wrote:
> On 2016/09/28 20:41:03, James Cook wrote:
> > This is fine, but just curious: does it need to change from
TYPE_PLATFORM_APP
> to
> > TYPE_DIALOG?
> 
> Nope! Just done to keep it a one-liner. Let me know if I should change it
back.

This is fine.

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2357143004/diff/190001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:109: if
(window->GetTitle() != l10n_util::GetStringUTF16(IDS_SETTINGS_TITLE))
On 2016/09/28 23:27:46, msw wrote:
> On 2016/09/28 20:41:03, James Cook wrote:
> > Aside: Ugh, aura::Window::SetTitle calls observers even if the title didn't
> > change. Bummer.
> 
> Acknowledged. Worth following up?

Yeah, I think it's worth fixing. There are only a handful of places that use
OnWindowTitleChanged. That would be a nice follow up CL.

https://codereview.chromium.org/2357143004/diff/210001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2357143004/diff/210001/ash/common/wm_window_p...
ash/common/wm_window_property.h:21: SHELF_ICON_ID,
I would also rename this SHELF_ICON_RESOURCE_ID to match the others. Sorry I
didn't suggest that before. And yes, it will make more lines wrap. :-(

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:89: public
ash::WmWindowTracker {
Neat, I wasn't familiar with this.

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc
(right):

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc:83:
ash::WmWindowProperty::SHELF_ITEM_TYPE,
Wait, don't we also need to set the icon ID when you set TYPE_DIALOG?

[msw, 2016-09-29 06:19:24]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2016-09-29 06:20:50]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2016-09-29 06:23:36]

Comments addressed; landing. I'll follow up with TODOs next.

https://codereview.chromium.org/2357143004/diff/210001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2357143004/diff/210001/ash/common/wm_window_p...
ash/common/wm_window_property.h:21: SHELF_ICON_ID,
On 2016/09/29 02:49:46, James Cook wrote:
> I would also rename this SHELF_ICON_RESOURCE_ID to match the others. Sorry I
> didn't suggest that before. And yes, it will make more lines wrap. :-(

Done. Clarity is more important than brevity :-)

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:89: public
ash::WmWindowTracker {
On 2016/09/29 02:49:46, James Cook wrote:
> Neat, I wasn't familiar with this.

Yeah, seems like commonly desired observer behavior.

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc
(right):

https://codereview.chromium.org/2357143004/diff/210001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc:83:
ash::WmWindowProperty::SHELF_ITEM_TYPE,
On 2016/09/29 02:49:46, James Cook wrote:
> Wait, don't we also need to set the icon ID when you set TYPE_DIALOG?

No, this is effectively just 'hiding' and 'showing' the item. The icon resource
id property value (and the window title) supplied by
BrowserStatusMonitor::SettingsWindowObserver are unchanged. I DCHECK'ed the
values here in an intermediary (but not uploaded?) patchset, but it doesn't seem
entirely worthwhile to include grit files and string utils just to dcheck those
values here.

[msw, 2016-09-29 06:23:42]

The CQ bit was checked by msw@chromium.org

[msw, 2016-09-29 06:23:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2357143004/#ps230001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-09-29 06:23:56]

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-29 08:10:29]

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

[commit-bot: I haz the power, 2016-09-29 08:10:30]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[msw, 2016-09-29 08:59:54]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-09-29 09:00:13]

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-29 09:40:29]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[commit-bot: I haz the power, 2016-09-29 09:40:30]

Committed patchset #10 (id:230001)

[commit-bot: I haz the power, 2016-09-29 09:42:27]

Description was changed from

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Support ShelfWindowWatcher via ShelfItem properties.

Show mash shelf items for open windows via ShelfWindowWatcher.

Decompose ash::ShelfItemDetails into several window properties.
Use the existing window title, with new WmWindow setters.
Add window icon and shelf item type properties.

Support "Settings" tooltip instead of "Google Chrome - Settings".
Use WmWindow directly in ShelfWindowWatcherItemDelegate.
(don't go through wm::WindowState helpers)

TODO: combine mash browser window items.
TODO: fix crash when clicking new shelf items, surfaced here.
TODO: fix mash settings window title and icon.
TODO: aura::Window::SetTitle calls observers always

BUG=634150
TEST=No cros regresssions; mash shelf shows items for open windows.
R=jamescook@chromium.org,tsepez@chromium.org

Committed: https://crrev.com/6ab444005ded90d3b70ad8bd086888307dcc924a
Cr-Commit-Position: refs/heads/master@{#421788}
==========

[commit-bot: I haz the power, 2016-09-29 09:42:28]

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