mash: Prerequisites for removing ShelfDelegate.

mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406, 698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
TBR=yusukes@chromium.org

Review-Url: https://codereview.chromium.org/2825533003
Cr-Commit-Position: refs/heads/master@{#465272}
Committed: https://chromium.googlesource.com/chromium/src/+/448103a0613cc1d0170c2fddf815c5d39ba411f7

[msw, 2017-04-17 21:19:48]

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

[commit-bot: I haz the power, 2017-04-17 21:20:01]

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, 2017-04-17 21:57:56]

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

[commit-bot: I haz the power, 2017-04-17 21:57:57]

Dry run: 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_...)

[msw, 2017-04-17 21:59:16]

Description was changed from

==========
Prerequisites for removing ShelfDelegate.

BUG=557406
==========

to

==========
Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Nix TestShelfDelegate's extra ShelfItem management and tracking.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

[msw, 2017-04-17 21:59:16]

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

[msw, 2017-04-17 22:02:40]

Hey James, please take a look; thanks! This CL is simpler than my exhaustive CL
description makes it seem :)

[msw, 2017-04-17 22:02:49]

Description was changed from

==========
Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Nix TestShelfDelegate's extra ShelfItem management and tracking.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

to

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Nix TestShelfDelegate's extra ShelfItem management and tracking.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

[msw, 2017-04-17 22:02:54]

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

[commit-bot: I haz the power, 2017-04-17 22:03:21]

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, 2017-04-17 22:51:44]

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

[commit-bot: I haz the power, 2017-04-17 22:51:45]

Dry run: 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_...)

[msw, 2017-04-17 23:40:42]

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

[commit-bot: I haz the power, 2017-04-17 23:41:18]

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

[msw, 2017-04-17 23:42:26]

Description was changed from

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Nix TestShelfDelegate's extra ShelfItem management and tracking.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

to

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

[James Cook, 2017-04-17 23:59:02]

Mostly questions. Also, you have a browser_tests failure.

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (left):

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:685: void ReplaceShelfDelegate() {
Nice to see junk like this going away.

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:1897: class ListMenuShelfItemDelegate : public
ash::ShelfItemDelegate {
nit: do these need namespace ash?

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

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:72: ash::ShelfItems::const_iterator item =
model->ItemByID(shelf_id);
nit: is ash:: needed?

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:73: return (item != model->items().end()) &&
item->type == TYPE_PINNED_APP;
nit: no extra parens

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:36: // shelf is created, to simulate
ChromeLauncherController's behavior.
Aside: Yuck. I wish we didn't need this.

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:39: ShelfInitializer() {
ash::Shell::Get()->AddShellObserver(this); }
nit: ash:: not needed?

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:45: // Do not override the custom initialization
performed by some unit tests.
Aside: Double yuck.

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager_unittest.cc:658: EXPECT_LT(window_x2,
window_x1);
Wait, how did this work before? Was it wrong before?

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (left):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:562: ui::wm::WINDOW_TYPE_PANEL,
This goes away because a panel can't have a transient child that is a panel?

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:137: (first_item_bounds.y() <
second_item_bounds.y()));
Do you have to check Y for item icons? Shouldn't they all be in a horizontal
line?

[commit-bot: I haz the power, 2017-04-18 00:09:52]

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

[commit-bot: I haz the power, 2017-04-18 00:09:52]

Dry run: This issue passed the CQ dry run.

[msw, 2017-04-18 01:07:05]

Test failures should be fixed (by patch set 3). Please take another look,
thanks!

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (left):

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:685: void ReplaceShelfDelegate() {
On 2017/04/17 23:59:02, James Cook wrote:
> Nice to see junk like this going away.

Acknowledged.

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:1897: class ListMenuShelfItemDelegate : public
ash::ShelfItemDelegate {
On 2017/04/17 23:59:02, James Cook wrote:
> nit: do these need namespace ash?

Done.

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

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:72: ash::ShelfItems::const_iterator item =
model->ItemByID(shelf_id);
On 2017/04/17 23:59:02, James Cook wrote:
> nit: is ash:: needed?

Done.

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:73: return (item != model->items().end()) &&
item->type == TYPE_PINNED_APP;
On 2017/04/17 23:59:02, James Cook wrote:
> nit: no extra parens

Done.

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:36: // shelf is created, to simulate
ChromeLauncherController's behavior.
On 2017/04/17 23:59:02, James Cook wrote:
> Aside: Yuck. I wish we didn't need this.

Acknowledged.

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:39: ShelfInitializer() {
ash::Shell::Get()->AddShellObserver(this); }
On 2017/04/17 23:59:02, James Cook wrote:
> nit: ash:: not needed?

Done.

https://codereview.chromium.org/2825533003/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:45: // Do not override the custom initialization
performed by some unit tests.
On 2017/04/17 23:59:02, James Cook wrote:
> Aside: Double yuck.

Acknowledged.

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager_unittest.cc:658: EXPECT_LT(window_x2,
window_x1);
On 2017/04/17 23:59:02, James Cook wrote:
> Wait, how did this work before? Was it wrong before?

TestShelfDelegate::AddShelfItem (the old test panel code path) always added
items to the end of the model's list. Now,
AshTestBase::CreateTestWindowInShellWithDelegateAndType sets the panel window's
kShelfItemTypeKey property, which triggers ShelfWindowWatcher's panel window
handling, and that inserts panel windows to the left of existing panel windows
(for a bottom-aligned shelf in LTR UI), which matches production behavior (yay!
unit tests and browser tests can expect the same thing!). My extra comment above
was meant to add a little clarity: "New shelf items for panels are inserted
before existing panel items."

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (left):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:562: ui::wm::WINDOW_TYPE_PANEL,
On 2017/04/17 23:59:02, James Cook wrote:
> This goes away because a panel can't have a transient child that is a panel?

That's the case as far as I know... Really, I removed this parameterized
instantiation of the test, because it fails 4 expectations for window bounds
while dragging (lines 513, 515, 525, and 527), likely related to the child panel
window attaching to the shelf in a different order (or perhaps at all...). I was
able to fix the parent panel window's bounds expectations by changing the order
of construction (creating the child before the parent, which requires explicit
destruction ordering by resetting the child's unique_ptr first at the bottom of
the test fixture), but I haven't been able to locally resolve the child panel's
bounds (I suspect it has to do with the child panel window actually attaching to
the shelf, whereas it didn't previously, but I'm not sure why...). I can dig
deeper or add a TODO and file a bug, if you think it's worthwhile.

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:137: (first_item_bounds.y() <
second_item_bounds.y()));
On 2017/04/17 23:59:02, James Cook wrote:
> Do you have to check Y for item icons? Shouldn't they all be in a horizontal
> line?

This function is also used for vertical shelf testing. (DragAlongShelfReorder is
called from PanelWindowResizerTest.DragReordersPanelsVertical)

[msw, 2017-04-18 01:07:14]

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

[commit-bot: I haz the power, 2017-04-18 01:07:35]

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, 2017-04-18 01:40:01]

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

[commit-bot: I haz the power, 2017-04-18 01:40:02]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-18 15:10:36]

LGTM. Thanks for splitting this into a separate patch.

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (left):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:562: ui::wm::WINDOW_TYPE_PANEL,
On 2017/04/18 01:07:05, msw wrote:
> On 2017/04/17 23:59:02, James Cook wrote:
> > This goes away because a panel can't have a transient child that is a panel?
> 
> That's the case as far as I know... Really, I removed this parameterized
> instantiation of the test, because it fails 4 expectations for window bounds
> while dragging (lines 513, 515, 525, and 527), likely related to the child
panel
> window attaching to the shelf in a different order (or perhaps at all...). I
was
> able to fix the parent panel window's bounds expectations by changing the
order
> of construction (creating the child before the parent, which requires explicit
> destruction ordering by resetting the child's unique_ptr first at the bottom
of
> the test fixture), but I haven't been able to locally resolve the child
panel's
> bounds (I suspect it has to do with the child panel window actually attaching
to
> the shelf, whereas it didn't previously, but I'm not sure why...). I can dig
> deeper or add a TODO and file a bug, if you think it's worthwhile.

I think this is OK. Frankly, I don't really understand transient windows all
that well.

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:137: (first_item_bounds.y() <
second_item_bounds.y()));
On 2017/04/18 01:07:05, msw wrote:
> On 2017/04/17 23:59:02, James Cook wrote:
> > Do you have to check Y for item icons? Shouldn't they all be in a horizontal
> > line?
> 
> This function is also used for vertical shelf testing. (DragAlongShelfReorder
is
> called from PanelWindowResizerTest.DragReordersPanelsVertical)

Can you either document that this is used for vertical shelf, or add a separate
function for testing vertical shelf that just does y?

[msw, 2017-04-18 16:43:46]

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2825533003/diff/20001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:137: (first_item_bounds.y() <
second_item_bounds.y()));
On 2017/04/18 15:10:36, James Cook wrote:
> On 2017/04/18 01:07:05, msw wrote:
> > On 2017/04/17 23:59:02, James Cook wrote:
> > > Do you have to check Y for item icons? Shouldn't they all be in a
horizontal
> > > line?
> > 
> > This function is also used for vertical shelf testing.
(DragAlongShelfReorder
> is
> > called from PanelWindowResizerTest.DragReordersPanelsVertical)
> 
> Can you either document that this is used for vertical shelf, or add a
separate
> function for testing vertical shelf that just does y?

Done.

[msw, 2017-04-18 16:43:54]

The CQ bit was checked by msw@chromium.org

[msw, 2017-04-18 16:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2825533003/#ps80001
(title: "Expand CheckWindowAndItemPlacement comment.")

[commit-bot: I haz the power, 2017-04-18 16:44:20]

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, 2017-04-18 16:52:17]

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

[commit-bot: I haz the power, 2017-04-18 16:52:18]

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

[msw, 2017-04-18 16:55:57]

Description was changed from

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
==========

to

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
TBR=yusukes@chromium.org
==========

[msw, 2017-04-18 16:55:57]

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

[msw, 2017-04-18 16:56:29]

+TBR yusukes@ for removing an unused include from:
chrome/browser/chromeos/arc/arc_session_manager.cc

[msw, 2017-04-18 16:56:39]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-04-18 16:56:58]

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, 2017-04-18 17:24:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492534599552590,
"parent_rev": "4682940d4a09ec69b133c495731b8b86d678dffa", "commit_rev":
"448103a0613cc1d0170c2fddf815c5d39ba411f7"}

[commit-bot: I haz the power, 2017-04-18 17:25:11]

Description was changed from

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
TBR=yusukes@chromium.org
==========

to

==========
mash: Prerequisites for removing ShelfDelegate.

This CL splits out some optionally prerequisite work from:
  https://codereview.chromium.org/2791463002/
Most changes are test-only, and shouldn't affect production.

Nix unused CreatePanel function in accelerator_controller_unittest.cc
Nix UserMetricsRecorderTest::CreateTestWindow, add ShelfModel items.
Nix unnecessary ShelfTooltipManagerTest and ShelfTest item delegates.

Make ShelfApplicationMenuModel work with a null item delegate.
(supports a simplification in the test file)

Make AshTestBase set the shelf item type property for test panels.
(uses ShelfWindowWatcher for test panels, similar to production)

Cleanup shelf_view_unittest.cc:
-Base classes on ash::ShelfItemDelegate, not TestShelfItemDelegate.
-Track the count of selections, rather than a bool & Reset().
-Add ShelfModel items directly instead of making windows for items.
-Nix TestShelfDelegateForShelfView, rely on TestShelfDelegate instead.
-Refactor helper functions for adding shelf items.

Make TestShelfDelegate directly manipulate ShelfModel items.
(behaves much more like CLC, the production ShelfDelegate impl)

Move ShelfInitializer from TestShelfDelegate to TestShellDelegate.
(TestShelfDelegate will be removed, but tests still need shelf init)

Exclude a few expectations for mash in PanelLayoutManagerTest.*

Refine PanelWindowResizerTest comments and expectations.
-Expect panel ordering like production (added right to left).
-Skip testing a panel as a transient child of a panel (odd, broken).

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes. Tests pass.
R=jamescook@chomium.org
TBR=yusukes@chromium.org

Review-Url: https://codereview.chromium.org/2825533003
Cr-Commit-Position: refs/heads/master@{#465272}
Committed:
https://chromium.googlesource.com/chromium/src/+/448103a0613cc1d0170c2fddf815...
==========

[commit-bot: I haz the power, 2017-04-18 17:25:12]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/448103a0613cc1d0170c2fddf815...