Remove docked windows entirely in M59.

Remove docked windows entirely in M59.

After we hid this feature behind a flag in M57, now it's time to remove it
completely.
This CL removes the feature, its tests, and the newly added flag.

BUG=668355
Review-Url: https://codereview.chromium.org/2700523004
Cr-Commit-Position: refs/heads/master@{#458508}
Committed: https://chromium.googlesource.com/chromium/src/+/cd7193637823047ec88226019bb1d987215a35db

[afakhry, 2017-02-16 21:50:29]

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

[commit-bot: I haz the power, 2017-02-16 21:51:53]

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-02-16 22:20:52]

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

[commit-bot: I haz the power, 2017-02-16 22:20:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[afakhry, 2017-02-16 22:21:51]

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

[commit-bot: I haz the power, 2017-02-16 22:22:57]

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-02-16 22:53:19]

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

[commit-bot: I haz the power, 2017-02-16 22:53:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[afakhry, 2017-02-22 02:33:23]

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

[commit-bot: I haz the power, 2017-02-22 02:34:05]

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

[afakhry, 2017-02-22 02:40:47]

afakhry@chromium.org changed reviewers:
+ mfomitchev@chromium.org, oshima@chromium.org

[afakhry, 2017-02-22 02:40:48]

Oshima-san, could you please review this CL? Thanks!

I have currently two failing tests in mash_unittests, but I'm not sure which
part of the removed code was relevant to them in the case of mash, especially
that they both pass in ash_unittests. +mfomitchev@ for advice.

The two failing tests are:
(https://cs.chromium.org/chromium/src/ash/common/drag_drop/drag_image_view_uni...)

[ RUN      ] DragDropImageTest.SetBoundsConsidersDragHintForTouch
../../ash/common/drag_drop/drag_image_view_unittest.cc:28: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 209,527 25x25
Expected: gfx::Rect(minimum_size)
Which is: 0,0 25x25
../../ash/common/drag_drop/drag_image_view_unittest.cc:33: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 209,527 25x25
Expected: gfx::Rect(new_pos, minimum_size)
Which is: 5,5 25x25
../../ash/common/drag_drop/drag_image_view_unittest.cc:40: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 207,526 26x26
Expected: large_bounds
Which is: 0,0 26x26
../../ash/common/drag_drop/drag_image_view_unittest.cc:44: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 207,526 26x26
Expected: gfx::Rect(new_pos, large_bounds.size())
Which is: 5,5 26x26


[ RUN      ] DragDropImageTest.SetBoundsIgnoresDragHintForMouse
../../ash/common/drag_drop/drag_image_view_unittest.cc:56: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 233,551 1x1
Expected: small_bounds
Which is: 0,0 1x1
../../ash/common/drag_drop/drag_image_view_unittest.cc:64: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 233,551 1x1
Expected: gfx::Rect(new_pos, small_bounds.size())
Which is: 5,5 1x1
../../ash/common/drag_drop/drag_image_view_unittest.cc:69: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 166,452 100x100
Expected: large_bounds
Which is: 0,0 100x100
../../ash/common/drag_drop/drag_image_view_unittest.cc:73: Failure
Value of: drag_image_view.GetBoundsInScreen()
  Actual: 166,452 100x100
Expected: gfx::Rect(new_pos, large_bounds.size())
Which is: 5,5 100x100

[commit-bot: I haz the power, 2017-02-22 02:57:14]

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

[commit-bot: I haz the power, 2017-02-22 02:57:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[oshima, 2017-02-22 06:49:06]

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

[oshima, 2017-02-22 06:49:07]

+varkha@

https://codereview.chromium.org/2700523004/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_table.cc:461: TOGGLE_FULLSCREEN,     
TOGGLE_MAXIMIZED,        WINDOW_POSITION_CENTER,
I think we still want to keep the orignal format.

https://codereview.chromium.org/2700523004/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace_controller.cc:65: };
can you remove the loop below?

https://codereview.chromium.org/2700523004/diff/20001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.cc:117: int kVisibleWindowContainerIds[] =
{kShellWindowId_PanelContainer,
nit: space betwee { and kShel...

https://codereview.chromium.org/2700523004/diff/20001/ash/public/cpp/shell_wi...
File ash/public/cpp/shell_window_ids.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/public/cpp/shell_wi...
ash/public/cpp/shell_window_ids.cc:20: // Docked, panel, launcher and status are
intentionally checked after other
update the comment

https://codereview.chromium.org/2700523004/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller_unittest.cc:1365: change_count_++;
looks like this isn't used?

https://codereview.chromium.org/2700523004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:221:
OnBeforePanelWidgetInit(use_default_bounds, &params, widget());
use_default_bounds is always true now. Can we remove it?

[oshima, 2017-02-22 06:50:01]

and you need to update native_widget_mac as well.

FAILED: obj/ui/views/views/native_widget_mac.o 
../../ui/views/widget/native_widget_mac.mm:423:14: error: no member named
'SHOW_STATE_DOCKED' in namespace 'ui'
    case ui::SHOW_STATE_DOCKED:
         ~~~~^
1 error generated.

[afakhry, 2017-02-22 17:41:11]

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

[commit-bot: I haz the power, 2017-02-22 17:41:46]

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-02-22 17:45:48]

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

[commit-bot: I haz the power, 2017-02-22 17:45:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[afakhry, 2017-02-22 18:20:32]

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

[commit-bot: I haz the power, 2017-02-22 18:21:07]

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-02-22 18:25:41]

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

[commit-bot: I haz the power, 2017-02-22 18:25:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[afakhry, 2017-02-22 18:34:25]

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

[commit-bot: I haz the power, 2017-02-22 18:35:23]

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-02-22 21:48:17]

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

[commit-bot: I haz the power, 2017-02-22 21:48:18]

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

[afakhry, 2017-02-22 22:04:44]

https://codereview.chromium.org/2700523004/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_table.cc:461: TOGGLE_FULLSCREEN,     
TOGGLE_MAXIMIZED,        WINDOW_POSITION_CENTER,
On 2017/02/22 06:49:07, oshima wrote:
> I think we still want to keep the orignal format.

Done.

https://codereview.chromium.org/2700523004/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace_controller.cc:65: };
On 2017/02/22 06:49:07, oshima wrote:
> can you remove the loop below?

Done.

https://codereview.chromium.org/2700523004/diff/20001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.cc:117: int kVisibleWindowContainerIds[] =
{kShellWindowId_PanelContainer,
On 2017/02/22 06:49:07, oshima wrote:
> nit: space betwee { and kShel...

Done. I don't know why git cl format produces this weird formatting.

https://codereview.chromium.org/2700523004/diff/20001/ash/public/cpp/shell_wi...
File ash/public/cpp/shell_window_ids.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/public/cpp/shell_wi...
ash/public/cpp/shell_window_ids.cc:20: // Docked, panel, launcher and status are
intentionally checked after other
On 2017/02/22 06:49:07, oshima wrote:
> update the comment

Done.

https://codereview.chromium.org/2700523004/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller_unittest.cc:1365: change_count_++;
On 2017/02/22 06:49:07, oshima wrote:
> looks like this isn't used?

Done.

https://codereview.chromium.org/2700523004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2700523004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:221:
OnBeforePanelWidgetInit(use_default_bounds, &params, widget());
On 2017/02/22 06:49:07, oshima wrote:
> use_default_bounds is always true now. Can we remove it?

Done.

[oshima, 2017-02-23 02:21:03]

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:530: // .
http://crbug.com/615033: EventGenerator doesn't work with ash. That can be
you meant "mash" ?

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:531: //   worked around
for the test, but no point until we decide on the first one.
I still don't quite Why this stopped working with your CL. Can you elaborate?

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:532: /*
I prefer #if 0 as it allows nesting.

https://codereview.chromium.org/2700523004/diff/80001/ash/wm/toplevel_window_...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/80001/ash/wm/toplevel_window_...
ash/wm/toplevel_window_event_handler_unittest.cc:180: }
Is this test still meaningful? I guess this was to catch the crash when
reparenting the dragging window between docked and normal container.

Does this still happen with panel container?

[afakhry, 2017-02-23 18:08:15]

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

[commit-bot: I haz the power, 2017-02-23 18:08:47]

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

[afakhry, 2017-02-23 18:08:54]

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:530: // .
http://crbug.com/615033: EventGenerator doesn't work with ash. That can be
On 2017/02/23 02:21:02, oshima wrote:
> you meant "mash" ?

I didn't make these changes. I think rebasing didn't work well in this patch.
This file was completely removed in https://codereview.chromium.org/2694623016,
and somehow I readded it when I was rebasing. Thanks for catching this.

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:531: //   worked around
for the test, but no point until we decide on the first one.
On 2017/02/23 02:21:02, oshima wrote:
> I still don't quite Why this stopped working with your CL. Can you elaborate?

Please ignore this file.

https://codereview.chromium.org/2700523004/diff/80001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_unittest.cc:532: /*
On 2017/02/23 02:21:02, oshima wrote:
> I prefer #if 0 as it allows nesting.

Please ignore this file.

https://codereview.chromium.org/2700523004/diff/80001/ash/wm/toplevel_window_...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/80001/ash/wm/toplevel_window_...
ash/wm/toplevel_window_event_handler_unittest.cc:180: }
On 2017/02/23 02:21:02, oshima wrote:
> Is this test still meaningful? I guess this was to catch the crash when
> reparenting the dragging window between docked and normal container.
> 
> Does this still happen with panel container?

It seems we still have reparenting with panels:
https://cs.chromium.org/chromium/src/ash/common/wm/panels/panel_window_resize...

However this test doesn't create a panel. So I changed it to make it create a
panel first.

[commit-bot: I haz the power, 2017-02-23 19:55:55]

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

[commit-bot: I haz the power, 2017-02-23 19:55:56]

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

[oshima, 2017-02-23 22:40:02]

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (left):

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:181:
client->SetCapture(nullptr);
I believe this (releasing the capture) was a part of test.

And you may need to apply similar patch to PanelWindowResizer I did in
https://codereview.chromium.org/1800793003 (but not sure).

Can you test if crbug.com/579226 repros with this patch (there are two repro
steps)? If it doesn't happen, then we probably don't need this test.

[afakhry, 2017-02-24 20:00:55]

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (left):

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:181:
client->SetCapture(nullptr);
On 2017/02/23 22:40:02, oshima wrote:
> I believe this (releasing the capture) was a part of test.
> 
> And you may need to apply similar patch to PanelWindowResizer I did in
> https://codereview.chromium.org/1800793003 (but not sure).
> 
> Can you test if crbug.com/579226 repros with this patch (there are two repro
> steps)? If it doesn't happen, then we probably don't need this test.

I tried both steps in issue 579226 and couldn't repro. Do you want me to remove
this test?

[mfomitchev, 2017-02-25 03:35:46]

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
This should be 19, not 10. 

Maybe we should be using 
kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer + 1
type of format here instead of assigning values directly to protect against this
type of mistake. Or perhaps even better - use "enum class"? If we didn't have
mash_unittests (where panels are likely half broken), we wouldn't even catch
this with tests.

Also another thing: Can you please remove DOCKED from the ShowState in
window_manager_constants_mojom?

[oshima, 2017-02-27 15:10:08]

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (left):

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:181:
client->SetCapture(nullptr);
On 2017/02/24 20:00:55, afakhry wrote:
> On 2017/02/23 22:40:02, oshima wrote:
> > I believe this (releasing the capture) was a part of test.
> > 
> > And you may need to apply similar patch to PanelWindowResizer I did in
> > https://codereview.chromium.org/1800793003 (but not sure).
> > 
> > Can you test if crbug.com/579226 repros with this patch (there are two repro
> > steps)? If it doesn't happen, then we probably don't need this test.
> 
> I tried both steps in issue 579226 and couldn't repro. Do you want me to
remove
> this test?

I think it does't happen because it's not panel window. I think the scenario may
not
be possible with a panel, and we're going to remove it anyway, so you can remove
it.

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
On 2017/02/25 03:35:46, mfomitchev wrote:
> This should be 19, not 10. 
> 
> Maybe we should be using 
> kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer + 1
> type of format here instead of assigning values directly to protect against
this
> type of mistake. Or perhaps even better - use "enum class"? If we didn't have
> mash_unittests (where panels are likely half broken), we wouldn't even catch
> this with tests.

I have slight preference to enum, although I think we should have had a test
that can catch it.

Can you update ExpectAllContainers in shell_unittest.cc to ensure that all
containers are different?

> 
> Also another thing: Can you please remove DOCKED from the ShowState in
> window_manager_constants_mojom?

[afakhry, 2017-02-27 17:43:29]

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

[afakhry, 2017-02-27 17:43:50]

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (left):

https://codereview.chromium.org/2700523004/diff/140001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:181:
client->SetCapture(nullptr);
On 2017/02/27 15:10:08, oshima wrote:
> On 2017/02/24 20:00:55, afakhry wrote:
> > On 2017/02/23 22:40:02, oshima wrote:
> > > I believe this (releasing the capture) was a part of test.
> > > 
> > > And you may need to apply similar patch to PanelWindowResizer I did in
> > > https://codereview.chromium.org/1800793003 (but not sure).
> > > 
> > > Can you test if crbug.com/579226 repros with this patch (there are two
repro
> > > steps)? If it doesn't happen, then we probably don't need this test.
> > 
> > I tried both steps in issue 579226 and couldn't repro. Do you want me to
> remove
> > this test?
> 
> I think it does't happen because it's not panel window. I think the scenario
may
> not
> be possible with a panel, and we're going to remove it anyway, so you can
remove
> it.

Done. removed the test.

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
On 2017/02/25 03:35:46, mfomitchev wrote:
> This should be 19, not 10. 
> 
> Maybe we should be using 
> kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer + 1
> type of format here instead of assigning values directly to protect against
this
> type of mistake. Or perhaps even better - use "enum class"? If we didn't have
> mash_unittests (where panels are likely half broken), we wouldn't even catch
> this with tests.
> 

Ouch!! Thanks for catching that!!
I'm glad too that we have a test to catch that.
Certainly an enum class would be better, but this CL is already too big, I don't
want to make it even bigger.

> Also another thing: Can you please remove DOCKED from the ShowState in
> window_manager_constants_mojom?

Done.

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
On 2017/02/27 15:10:08, oshima wrote:
> On 2017/02/25 03:35:46, mfomitchev wrote:
> > This should be 19, not 10. 
> > 
> > Maybe we should be using 
> > kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer +
1
> > type of format here instead of assigning values directly to protect against
> this
> > type of mistake. Or perhaps even better - use "enum class"? If we didn't
have
> > mash_unittests (where panels are likely half broken), we wouldn't even catch
> > this with tests.
> 
> I have slight preference to enum, although I think we should have had a test
> that can catch it.
> 

I'd like to convert them to an enum too, but I think I should do this in a
different CL.

> Can you update ExpectAllContainers in shell_unittest.cc to ensure that all
> containers are different?
> 

Done.

[commit-bot: I haz the power, 2017-02-27 17:44:26]

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-02-27 19:15:23]

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

[commit-bot: I haz the power, 2017-02-27 19:15:24]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-02-28 00:59:03]

Thanks for doing this! A few places where I think we can remove more and one
place where I don't think we can remove the persistent state handler without
risking breaking saved profiles.

https://codereview.chromium.org/2700523004/diff/180001/ash/BUILD.gn
File ash/BUILD.gn (left):

https://codereview.chromium.org/2700523004/diff/180001/ash/BUILD.gn#oldcode469
ash/BUILD.gn:469: "common/wm/dock/docked_window_layout_manager.h",
Please make sure that all those files are removed.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:802: return
CanHandleWindowSnapOrDock();
Rename to CanHandleWindowSnap ?

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:1066:
HandleWindowSnapOrDock(action);
Rename to HandleWindowSnap ?

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

https://codereview.chromium.org/2700523004/diff/180001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:222:
TEST_F(ShelfWindowWatcherTest, DockWindow) {
Do we still need this test?

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/dock/doc...
File ash/common/wm/dock/dock_types.h (left):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/dock/doc...
ash/common/wm/dock/dock_types.h:25: // User action recorded for use in UMA
histograms.
It may be possible to remove all "Ash.Dock.*" histograms now. See
https://codereview.chromium.org/45343003/diff/510001/tools/metrics/histograms...
for what can be removed.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_window_resizer.cc:886: gfx::Rect
phantom_bounds =
Can be const.

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:113: // This list is needed to to validate we
have no duplicate IDs.
nit: double 'to'.

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:142: const size_t kAllShellContainerIdsSize =
arraysize(kAllShellContainerIds);
Can this size and '#include "base/macros.h' be moved to the unit test?

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:539: // Window is wide
enough not to get docked right away.
Do we still care about the window width?

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:588: // Window is wide
enough not to get docked right away.
ditto.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1488: // Make the window
wider than maximum docked width.
No need to refer to docking? Is this still necessary for this test?

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1513: // With min size
defined, we get no phantom window for docking.
Is setting a minimum size still affecting this test?

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace_contr...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace_contr...
ash/wm/workspace_controller_unittest.cc:1369: class
WorkspaceControllerTestDragging : public WorkspaceControllerTest {
Is this class still necessary?

https://codereview.chromium.org/2700523004/diff/180001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6
I don't think you can remove DOCKED_DEPRECATED here without breaking the profile
history. You can remove the handling of docked state in
ShowStateToPersistedShowState() (since no window will ever be docked) but I
think you need to leave the handler for the deprecated state in
PersistedShowStateToShowState() and simply return ui:SHOW_STATE_NORMAL there in
old line 171.

[varkha, 2017-02-28 01:32:22]

A few more places that you may be able to remove some code or reference to
"docked":

https://cs.chromium.org/chromium/src/ash/common/wm/window_state_observer.h?ty...
https://cs.chromium.org/chromium/src/chrome/common/extensions/api/windows.jso...
https://cs.chromium.org/chromium/src/ash/common/wm/fullscreen_window_finder.c...

I could have missed something still so it is probably worth it to search for
"docked" (this would have many false positives but I don't have a better idea
other than maybe look through some old CLs that introduced docking state).

https://codereview.chromium.org/2700523004/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:220:
OnBeforePanelWidgetInit(true /* use_default_bounds */, &params, widget());
Do we still need the use_default_bounds parameter for OnBeforePanelWidgetInit()?
I think it was introduced only for docking support here -
https://codereview.chromium.org/1056793006.

[varkha, 2017-02-28 17:20:32]

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
On 2017/02/27 15:10:08, oshima wrote:
> On 2017/02/25 03:35:46, mfomitchev wrote:
> > This should be 19, not 10. 
> > 
> > Maybe we should be using 
> > kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer +
1
> > type of format here instead of assigning values directly to protect against
> this
> > type of mistake. Or perhaps even better - use "enum class"? If we didn't
have
> > mash_unittests (where panels are likely half broken), we wouldn't even catch
> > this with tests.
> 
> I have slight preference to enum, although I think we should have had a test
> that can catch it.
> 
> Can you update ExpectAllContainers in shell_unittest.cc to ensure that all
> containers are different?
> 
> > 
> > Also another thing: Can you please remove DOCKED from the ShowState in
> > window_manager_constants_mojom?
> 

I would also prefer to make it enum and have a compiler enforce the ordering 
and uniqueness instead of (or in addition to) relying on a test.

[afakhry, 2017-03-09 22:22:00]

Description was changed from

==========
Remove docked windows entirely in M58.

After we hid this feature behind a flag in M57, now it's time to remove it
completely.
This CL removes the feature, its tests, and the newly added flag.

BUG=668355
==========

to

==========
Remove docked windows entirely in M59.

After we hid this feature behind a flag in M57, now it's time to remove it
completely.
This CL removes the feature, its tests, and the newly added flag.

BUG=668355
==========

[afakhry, 2017-03-09 22:27:58]

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

[afakhry, 2017-03-09 22:28:27]

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/160001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:89: const int32_t
kShellWindowId_DragImageAndTooltipContainer = 10;
On 2017/02/28 17:20:32, varkha wrote:
> On 2017/02/27 15:10:08, oshima wrote:
> > On 2017/02/25 03:35:46, mfomitchev wrote:
> > > This should be 19, not 10. 
> > > 
> > > Maybe we should be using 
> > > kShellWindowId_DragImageAndTooltipContainer = kShellWindowId_MenuContainer
+
> 1
> > > type of format here instead of assigning values directly to protect
against
> > this
> > > type of mistake. Or perhaps even better - use "enum class"? If we didn't
> have
> > > mash_unittests (where panels are likely half broken), we wouldn't even
catch
> > > this with tests.
> > 
> > I have slight preference to enum, although I think we should have had a test
> > that can catch it.
> > 
> > Can you update ExpectAllContainers in shell_unittest.cc to ensure that all
> > containers are different?
> > 
> > > 
> > > Also another thing: Can you please remove DOCKED from the ShowState in
> > > window_manager_constants_mojom?
> > 
> 
> I would also prefer to make it enum and have a compiler enforce the ordering 
> and uniqueness instead of (or in addition to) relying on a test.

That's also my preference, but let me know If I should do this as part of this
CL?

https://codereview.chromium.org/2700523004/diff/180001/ash/BUILD.gn
File ash/BUILD.gn (left):

https://codereview.chromium.org/2700523004/diff/180001/ash/BUILD.gn#oldcode469
ash/BUILD.gn:469: "common/wm/dock/docked_window_layout_manager.h",
On 2017/02/28 00:59:02, varkha wrote:
> Please make sure that all those files are removed.

Done.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:802: return
CanHandleWindowSnapOrDock();
On 2017/02/28 00:59:02, varkha wrote:
> Rename to CanHandleWindowSnap ?

Done.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:1066:
HandleWindowSnapOrDock(action);
On 2017/02/28 00:59:02, varkha wrote:
> Rename to HandleWindowSnap ?

Done.

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

https://codereview.chromium.org/2700523004/diff/180001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:222:
TEST_F(ShelfWindowWatcherTest, DockWindow) {
On 2017/02/28 00:59:02, varkha wrote:
> Do we still need this test?

Nope. Removed.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/dock/doc...
File ash/common/wm/dock/dock_types.h (left):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/dock/doc...
ash/common/wm/dock/dock_types.h:25: // User action recorded for use in UMA
histograms.
On 2017/02/28 00:59:02, varkha wrote:
> It may be possible to remove all "Ash.Dock.*" histograms now. See
>
https://codereview.chromium.org/45343003/diff/510001/tools/metrics/histograms...
> for what can be removed.

From what I understand, histogram.xml should not be changed even if we remove
the histograms themselves. The entires in histograms.xml are still needed to
decode histograms from previous versions.

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_window_resizer.cc:886: gfx::Rect
phantom_bounds =
On 2017/02/28 00:59:02, varkha wrote:
> Can be const.

Done.

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:113: // This list is needed to to validate we
have no duplicate IDs.
On 2017/02/28 00:59:02, varkha wrote:
> nit: double 'to'.

Done.

https://codereview.chromium.org/2700523004/diff/180001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:142: const size_t kAllShellContainerIdsSize =
arraysize(kAllShellContainerIds);
On 2017/02/28 00:59:02, varkha wrote:
> Can this size and '#include "base/macros.h' be moved to the unit test?

Done.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:539: // Window is wide
enough not to get docked right away.
On 2017/02/28 00:59:02, varkha wrote:
> Do we still care about the window width?

I think we need to keep the bounds just to match the expectations of the bounds
below, but remove the referral to docked.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:588: // Window is wide
enough not to get docked right away.
On 2017/02/28 00:59:03, varkha wrote:
> ditto.

Just as above.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1488: // Make the window
wider than maximum docked width.
On 2017/02/28 00:59:02, varkha wrote:
> No need to refer to docking? Is this still necessary for this test?

Removed.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace/works...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1513: // With min size
defined, we get no phantom window for docking.
On 2017/02/28 00:59:02, varkha wrote:
> Is setting a minimum size still affecting this test?

Removed.

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace_contr...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/ash/wm/workspace_contr...
ash/wm/workspace_controller_unittest.cc:1369: class
WorkspaceControllerTestDragging : public WorkspaceControllerTest {
On 2017/02/28 00:59:03, varkha wrote:
> Is this class still necessary?

Nope, removed.

https://codereview.chromium.org/2700523004/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:220:
OnBeforePanelWidgetInit(true /* use_default_bounds */, &params, widget());
On 2017/02/28 01:32:22, varkha wrote:
> Do we still need the use_default_bounds parameter for
OnBeforePanelWidgetInit()?
> I think it was introduced only for docking support here -
> https://codereview.chromium.org/1056793006.

No, I don't think we still need that. I removed it.

https://codereview.chromium.org/2700523004/diff/180001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/2700523004/diff/180001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6
On 2017/02/28 00:59:03, varkha wrote:
> I don't think you can remove DOCKED_DEPRECATED here without breaking the
profile
> history. You can remove the handling of docked state in
> ShowStateToPersistedShowState() (since no window will ever be docked) but I
> think you need to leave the handler for the deprecated state in
> PersistedShowStateToShowState() and simply return ui:SHOW_STATE_NORMAL there
in
> old line 171.

Done.

https://codereview.chromium.org/2700523004/diff/220001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/220001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
I wonder if this change will break some extensions?

[commit-bot: I haz the power, 2017-03-09 22:29:04]

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-03-10 00:34:29]

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

[commit-bot: I haz the power, 2017-03-10 00:34:31]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-10 01:18:29]

drive-by comment

https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:113: const int32_t kAllShellContainerIds[] = {
With the introduction of enums, this will go away, right? Instead of adding this
with this CL, and removing it later, would it be possible to convert to enums
first, and then land this CL? (that would also make it easier to do a revert if
needed for some reason)

[afakhry, 2017-03-14 02:07:30]

https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
File ash/public/cpp/shell_window_ids.h (right):

https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
ash/public/cpp/shell_window_ids.h:113: const int32_t kAllShellContainerIds[] = {
On 2017/03/10 01:18:28, sadrul wrote:
> With the introduction of enums, this will go away, right? Instead of adding
this
> with this CL, and removing it later, would it be possible to convert to enums
> first, and then land this CL? (that would also make it easier to do a revert
if
> needed for some reason)

I have a CL (https://codereview.chromium.org/2751523003) to do the conversion to
enum. As you can see it's quit large and it touches a lot of files, and I think
if I land it first, rebasing this CL on that other CL will be a such a pain in
the neck! What do you suggest?

[oshima, 2017-03-14 02:34:52]

On 2017/03/14 02:07:30, afakhry wrote:
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> File ash/public/cpp/shell_window_ids.h (right):
> 
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> ash/public/cpp/shell_window_ids.h:113: const int32_t kAllShellContainerIds[] =
{
> On 2017/03/10 01:18:28, sadrul wrote:
> > With the introduction of enums, this will go away, right? Instead of adding
> this
> > with this CL, and removing it later, would it be possible to convert to
enums
> > first, and then land this CL? (that would also make it easier to do a revert
> if
> > needed for some reason)
> 
> I have a CL (https://codereview.chromium.org/2751523003) to do the conversion
to
> enum. As you can see it's quit large and it touches a lot of files, and I
think
> if I land it first, rebasing this CL on that other CL will be a such a pain in
> the neck! What do you suggest?

I'm fine to land this first if you can land the enum change next.

[afakhry, 2017-03-15 03:06:10]

Hi all,

Friendly ping. I need to get this out of the way soon. Thanks!

[varkha, 2017-03-15 19:51:16]

lgtm with a couple nits. Thanks again for doing this!

https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:492:
TEST_F(AcceleratorControllerTest, WindowSnapWithoutDocking) {
nit: I suggest renaming this, maybe something about "TestRepeatedSnap".

https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:322: return
!!WmShell::Get()->GetActiveWindow();
nit: I don't remember when but I had a comment once in one of my CLs about not
using !!. Maybe use explicit != nullptr here.

[afakhry, 2017-03-15 20:25:51]

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

[commit-bot: I haz the power, 2017-03-15 20:26:21]

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

[afakhry, 2017-03-15 20:27:24]

afakhry@chromium.org changed reviewers:
+ sky@chromium.org

[afakhry, 2017-03-15 20:27:26]

+sky@ for OWNERS review.

https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:492:
TEST_F(AcceleratorControllerTest, WindowSnapWithoutDocking) {
On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> nit: I suggest renaming this, maybe something about "TestRepeatedSnap".

Done.

https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:322: return
!!WmShell::Get()->GetActiveWindow();
On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> nit: I don't remember when but I had a comment once in one of my CLs about not
> using !!. Maybe use explicit != nullptr here.

Done.

[commit-bot: I haz the power, 2017-03-15 21:37:35]

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

[commit-bot: I haz the power, 2017-03-15 21:37:36]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-15 23:12:53]

On 2017/03/15 20:27:26, afakhry wrote:
> +sky@ for OWNERS review.

This is a big patch! I'm happy to see code nuked! Can you clarify what files
(directories) you need me to review? You have Oshima, which is an owner of ash,
so maybe you need me solely for chrome/ and ui/views?

>
https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
> File ash/accelerators/accelerator_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
> ash/accelerators/accelerator_controller_unittest.cc:492:
> TEST_F(AcceleratorControllerTest, WindowSnapWithoutDocking) {
> On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> > nit: I suggest renaming this, maybe something about "TestRepeatedSnap".
> 
> Done.
> 
>
https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
> File ash/common/accelerators/accelerator_controller.cc (right):
> 
>
https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
> ash/common/accelerators/accelerator_controller.cc:322: return
> !!WmShell::Get()->GetActiveWindow();
> On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> > nit: I don't remember when but I had a comment once in one of my CLs about
not
> > using !!. Maybe use explicit != nullptr here.
> 
> Done.

[afakhry, 2017-03-15 23:25:53]

On 2017/03/15 23:12:53, sky wrote:
> On 2017/03/15 20:27:26, afakhry wrote:
> > +sky@ for OWNERS review.
> 
> This is a big patch! I'm happy to see code nuked! Can you clarify what files
> (directories) you need me to review? You have Oshima, which is an owner of
ash,
> so maybe you need me solely for chrome/ and ui/views?
> 

Yes, chrome/ and ui/views. Thank you very much!

> >
>
https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
> > File ash/accelerators/accelerator_controller_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2700523004/diff/240001/ash/accelerators/accel...
> > ash/accelerators/accelerator_controller_unittest.cc:492:
> > TEST_F(AcceleratorControllerTest, WindowSnapWithoutDocking) {
> > On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> > > nit: I suggest renaming this, maybe something about "TestRepeatedSnap".
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
> > File ash/common/accelerators/accelerator_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2700523004/diff/240001/ash/common/accelerator...
> > ash/common/accelerators/accelerator_controller.cc:322: return
> > !!WmShell::Get()->GetActiveWindow();
> > On 2017/03/15 19:51:16, varkha (OOO Mar 15) wrote:
> > > nit: I don't remember when but I had a comment once in one of my CLs about
> not
> > > using !!. Maybe use explicit != nullptr here.
> > 
> > Done.

[oshima, 2017-03-15 23:25:57]

my bits lgtm

[sky, 2017-03-16 04:24:24]

Pleaes also make sure you remove stale includes. For example, I suspect there
are a bunch of stale includes in
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
What happens if docked is still used?

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6,
I think you should keep END at 7, and after dock, but change the assert to -1
with a description. My rationale is if you keep end here it's all to easy to
miss it and then make the next value 7, which we wouldn't want to do.

[varkha, 2017-03-16 14:13:54]

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/16 04:24:23, sky wrote:
> What happens if docked is still used?

I think because the state is never set to DOCKED by the C code, this string is
never reported and doesn't need to be in validator. I might be wrong here.

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6,
On 2017/03/16 04:24:23, sky wrote:
> I think you should keep END at 7, and after dock, but change the assert to -1
> with a description. My rationale is if you keep end here it's all to easy to
> miss it and then make the next value 7, which we wouldn't want to do.

Or maybe set it to 8 to make detached and docked deprecated states handling
similar and shift assert by -2 with a comment that deprecated states at the end
of enum are not represented in ui::WindowShowState.

[afakhry, 2017-03-16 17:49:33]

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

[commit-bot: I haz the power, 2017-03-16 17:50:12]

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

[afakhry, 2017-03-16 17:50:22]

afakhry@chromium.org changed reviewers:
+ mkwst@chromium.org

[afakhry, 2017-03-16 17:50:24]

Removed many unnecessary includes

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/16 04:24:23, sky wrote:
> What happens if docked is still used?

I also think like what varkha said below, but adding +mkwst@chromium.org to make
sure and also for OWNERS review of this file.

https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/16 14:13:54, varkha (OOO Mar 15) wrote:
> On 2017/03/16 04:24:23, sky wrote:
> > What happens if docked is still used?
> 
> I think because the state is never set to DOCKED by the C code, this string is
> never reported and doesn't need to be in validator. I might be wrong here.

Acknowledged.

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6,
On 2017/03/16 14:13:54, varkha (OOO Mar 15) wrote:
> On 2017/03/16 04:24:23, sky wrote:
> > I think you should keep END at 7, and after dock, but change the assert to
-1
> > with a description. My rationale is if you keep end here it's all to easy to
> > miss it and then make the next value 7, which we wouldn't want to do.
> 
> Or maybe set it to 8 to make detached and docked deprecated states handling
> similar and shift assert by -2 with a comment that deprecated states at the
end
> of enum are not represented in ui::WindowShowState.

Done.

https://codereview.chromium.org/2700523004/diff/260001/components/sessions/co...
components/sessions/core/session_service_commands.cc:116:
PERSISTED_SHOW_STATE_END = 6,
On 2017/03/16 04:24:23, sky wrote:
> I think you should keep END at 7, and after dock, but change the assert to -1
> with a description. My rationale is if you keep end here it's all to easy to
> miss it and then make the next value 7, which we wouldn't want to do.

Done, but also followed varkha's comments below.

[commit-bot: I haz the power, 2017-03-16 19:04:48]

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

[commit-bot: I haz the power, 2017-03-16 19:04:51]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-16 20:26:08]

LGTM, but make sure you want for feedback on the json change.

[afakhry, 2017-03-16 22:50:37]

afakhry@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[afakhry, 2017-03-16 22:50:39]

Thanks, sky!

+rdevlin.cronin@ for chrome/common/extensions/api/windows.json. Devlin, please
take a look, thank you!

[afakhry, 2017-03-16 22:53:05]

afakhry@chromium.org changed reviewers:
+ dcheng@chromium.org

[afakhry, 2017-03-16 22:53:07]

+dcheng@ for  services/ui/public/interfaces/window_manager_constants.mojom.
Thank you!

[Devlin, 2017-03-17 02:24:40]

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
I don't see anywhere in extension docs that we publicly mentioned that we were
deprecating docked windows, and we allow this as a possible state in
windows.create().  This means that any function that did windows.create({state:
'docked'}) will suddenly receive a runtime error and fail to function.  Did we
announce this somewhere that I missed?  If not, we may need to leave this for a
bit with a deprecated warning.

[varkha, 2017-03-17 03:39:44]

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/17 02:24:39, Devlin wrote:
> I don't see anywhere in extension docs that we publicly mentioned that we were
> deprecating docked windows, and we allow this as a possible state in
> windows.create().  This means that any function that did
windows.create({state:
> 'docked'}) will suddenly receive a runtime error and fail to function.  Did we
> announce this somewhere that I missed?  If not, we may need to leave this for
a
> bit with a deprecated warning.

Would it not get silently converted to WINDOW_STATE_NONE here [1] and then
treated as ui::SHOW_STATE_DEFAULT here [2]?

[1] -
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
[2] -
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...

[dcheng, 2017-03-17 05:29:20]

rs lgtm for mojom change (but please make sure devlin's concerns are addressed
before landing)

[Mike West, 2017-03-17 12:33:25]

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

[Mike West, 2017-03-17 12:33:27]

On 2017/03/16 at 17:50:24, afakhry wrote:
>
https://codereview.chromium.org/2700523004/diff/260001/chrome/common/extensio...
> chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
> On 2017/03/16 04:24:23, sky wrote:
> > What happens if docked is still used?
> 
> I also think like what varkha said below, but adding +mkwst@chromium.org to
make sure and also for OWNERS review of this file.

I'm only in that OWNERS file for documentation review; I'll defer completely to
rdevlin.cronin@. :)

[Devlin, 2017-03-17 15:06:27]

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/17 03:39:44, varkha (OOO Mar 15) wrote:
> On 2017/03/17 02:24:39, Devlin wrote:
> > I don't see anywhere in extension docs that we publicly mentioned that we
were
> > deprecating docked windows, and we allow this as a possible state in
> > windows.create().  This means that any function that did
> windows.create({state:
> > 'docked'}) will suddenly receive a runtime error and fail to function.  Did
we
> > announce this somewhere that I missed?  If not, we may need to leave this
for
> a
> > bit with a deprecated warning.
> 
> Would it not get silently converted to WINDOW_STATE_NONE here [1] and then
> treated as ui::SHOW_STATE_DEFAULT here [2]?
> 
> [1] -
>
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
> [2] -
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...

No - the javascript bindings verify that the passed string is a valid option of
those specified in the "enum" list.  So trying to pass in "docked" would result
in an error (you can see the effect now by doing, e.g.
`chrome.windows.create({state: 'notastate'})`).

[afakhry, 2017-03-17 17:43:23]

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/17 15:06:27, Devlin wrote:
> On 2017/03/17 03:39:44, varkha (OOO Mar 15) wrote:
> > On 2017/03/17 02:24:39, Devlin wrote:
> > > I don't see anywhere in extension docs that we publicly mentioned that we
> were
> > > deprecating docked windows, and we allow this as a possible state in
> > > windows.create().  This means that any function that did
> > windows.create({state:
> > > 'docked'}) will suddenly receive a runtime error and fail to function. 
Did
> we
> > > announce this somewhere that I missed?  If not, we may need to leave this
> for
> > a
> > > bit with a deprecated warning.
> > 
> > Would it not get silently converted to WINDOW_STATE_NONE here [1] and then
> > treated as ui::SHOW_STATE_DEFAULT here [2]?
> > 
> > [1] -
> >
>
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
> > [2] -
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...
> 
> No - the javascript bindings verify that the passed string is a valid option
of
> those specified in the "enum" list.  So trying to pass in "docked" would
result
> in an error (you can see the effect now by doing, e.g.
> `chrome.windows.create({state: 'notastate'})`).

Thanks Devlin for catching that!
I re-added the "docked" entry. But I have a couple of questions:
- Where do we update the documentation to mention docked windows will be
deprecated?
- Do we also need to revert the changes in files:
    window_manager_constants.mojom,
    tabs_api.cc
  in this CL?

[varkha, 2017-03-17 17:52:47]

> - Do we also need to revert the changes in files:
>     window_manager_constants.mojom,
>     tabs_api.cc
>   in this CL?

I think yes for tabs_api.cc to handle all the auto-generated bindings but no for
window_manager_constants.mojom which is handling state internal to mus.

[sky, 2017-03-17 19:16:32]

window_manager_constants.mojom is entirely internal to chrome, so you
can update it.

  -Scott

On Fri, Mar 17, 2017 at 10:43 AM,  <afakhry@chromium.org> wrote:
>
>
https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
> File chrome/common/extensions/api/windows.json (left):
>
>
https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
> chrome/common/extensions/api/windows.json:23: "enum": ["normal",
> "minimized", "maximized", "fullscreen", "docked"]
> On 2017/03/17 15:06:27, Devlin wrote:
>> On 2017/03/17 03:39:44, varkha (OOO Mar 15) wrote:
>> > On 2017/03/17 02:24:39, Devlin wrote:
>> > > I don't see anywhere in extension docs that we publicly mentioned
> that we
>> were
>> > > deprecating docked windows, and we allow this as a possible state
> in
>> > > windows.create(). This means that any function that did
>> > windows.create({state:
>> > > 'docked'}) will suddenly receive a runtime error and fail to
> function. Did
>> we
>> > > announce this somewhere that I missed? If not, we may need to
> leave this
>> for
>> > a
>> > > bit with a deprecated warning.
>> >
>> > Would it not get silently converted to WINDOW_STATE_NONE here [1]
> and then
>> > treated as ui::SHOW_STATE_DEFAULT here [2]?
>> >
>> > [1] -
>> >
>>
>
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
>> > [2] -
>> >
>>
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...
>>
>> No - the javascript bindings verify that the passed string is a valid
> option of
>> those specified in the "enum" list. So trying to pass in "docked"
> would result
>> in an error (you can see the effect now by doing, e.g.
>> `chrome.windows.create({state: 'notastate'})`).
>
> Thanks Devlin for catching that!
> I re-added the "docked" entry. But I have a couple of questions:
> - Where do we update the documentation to mention docked windows will be
> deprecated?
> - Do we also need to revert the changes in files:
> window_manager_constants.mojom,
> tabs_api.cc
> in this CL?
>
> https://codereview.chromium.org/2700523004/

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

[Devlin, 2017-03-17 19:27:00]

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/17 17:43:23, afakhry wrote:
> On 2017/03/17 15:06:27, Devlin wrote:
> > On 2017/03/17 03:39:44, varkha (OOO Mar 15) wrote:
> > > On 2017/03/17 02:24:39, Devlin wrote:
> > > > I don't see anywhere in extension docs that we publicly mentioned that
we
> > were
> > > > deprecating docked windows, and we allow this as a possible state in
> > > > windows.create().  This means that any function that did
> > > windows.create({state:
> > > > 'docked'}) will suddenly receive a runtime error and fail to function. 
> Did
> > we
> > > > announce this somewhere that I missed?  If not, we may need to leave
this
> > for
> > > a
> > > > bit with a deprecated warning.
> > > 
> > > Would it not get silently converted to WINDOW_STATE_NONE here [1] and then
> > > treated as ui::SHOW_STATE_DEFAULT here [2]?
> > > 
> > > [1] -
> > >
> >
>
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
> > > [2] -
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...
> > 
> > No - the javascript bindings verify that the passed string is a valid option
> of
> > those specified in the "enum" list.  So trying to pass in "docked" would
> result
> > in an error (you can see the effect now by doing, e.g.
> > `chrome.windows.create({state: 'notastate'})`).
> 
> Thanks Devlin for catching that!
> I re-added the "docked" entry. But I have a couple of questions:
> - Where do we update the documentation to mention docked windows will be
> deprecated?
> - Do we also need to revert the changes in files:
>     window_manager_constants.mojom,
>     tabs_api.cc
>   in this CL?

This file actually generates the public documentation.  Deprecating enums isn't
quite as graceful as other properties, unfortunately.  But something like this
should work:

"enum": [{
  "name": "normal",
  "description": <description>
}, {
  "name": "minimized",
  "description": <description>
}, {
  "name": "maximized",
  "description": <description>
}, {
  "name": "fullscreen",
  "description": <description>
}, {
  "name": "docked",
  "description": <i>Deprecated since Chrome M59.</i>  Docked windows are no
longer supported.  This state will be converted to \"normal\"."
}],

After that, you should be able to preview your changes with
src/ $ python chrome/common/extensions/docs/server2/preview.py
and visiting http://localhost:8000/extensions/windows#type-WindowState.

And you will need to revert the tabs_api.cc changes, since this file also
generates those windows:: enums. :)

Ideally, we can also probably add UMA to this to see how often "docked" is
passed in as a parameter, and use that data to determine if it is safe to remove
it entirely.

[afakhry, 2017-03-17 23:34:42]

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

[afakhry, 2017-03-17 23:34:59]

Devlin, PTAL. Thanks!

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/windows.json (left):

https://codereview.chromium.org/2700523004/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/windows.json:23: "enum": ["normal", "minimized",
"maximized", "fullscreen", "docked"]
On 2017/03/17 19:27:00, Devlin wrote:
> On 2017/03/17 17:43:23, afakhry wrote:
> > On 2017/03/17 15:06:27, Devlin wrote:
> > > On 2017/03/17 03:39:44, varkha (OOO Mar 15) wrote:
> > > > On 2017/03/17 02:24:39, Devlin wrote:
> > > > > I don't see anywhere in extension docs that we publicly mentioned that
> we
> > > were
> > > > > deprecating docked windows, and we allow this as a possible state in
> > > > > windows.create().  This means that any function that did
> > > > windows.create({state:
> > > > > 'docked'}) will suddenly receive a runtime error and fail to function.

> > Did
> > > we
> > > > > announce this somewhere that I missed?  If not, we may need to leave
> this
> > > for
> > > > a
> > > > > bit with a deprecated warning.
> > > > 
> > > > Would it not get silently converted to WINDOW_STATE_NONE here [1] and
then
> > > > treated as ui::SHOW_STATE_DEFAULT here [2]?
> > > > 
> > > > [1] -
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/a...
> > > > [2] -
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...
> > > 
> > > No - the javascript bindings verify that the passed string is a valid
option
> > of
> > > those specified in the "enum" list.  So trying to pass in "docked" would
> > result
> > > in an error (you can see the effect now by doing, e.g.
> > > `chrome.windows.create({state: 'notastate'})`).
> > 
> > Thanks Devlin for catching that!
> > I re-added the "docked" entry. But I have a couple of questions:
> > - Where do we update the documentation to mention docked windows will be
> > deprecated?
> > - Do we also need to revert the changes in files:
> >     window_manager_constants.mojom,
> >     tabs_api.cc
> >   in this CL?
> 
> This file actually generates the public documentation.  Deprecating enums
isn't
> quite as graceful as other properties, unfortunately.  But something like this
> should work:
> 
> "enum": [{
>   "name": "normal",
>   "description": <description>
> }, {
>   "name": "minimized",
>   "description": <description>
> }, {
>   "name": "maximized",
>   "description": <description>
> }, {
>   "name": "fullscreen",
>   "description": <description>
> }, {
>   "name": "docked",
>   "description": <i>Deprecated since Chrome M59.</i>  Docked windows are no
> longer supported.  This state will be converted to \"normal\"."
> }],
> 
> After that, you should be able to preview your changes with
> src/ $ python chrome/common/extensions/docs/server2/preview.py
> and visiting http://localhost:8000/extensions/windows#type-WindowState.
> 
> And you will need to revert the tabs_api.cc changes, since this file also
> generates those windows:: enums. :)
> 
> Ideally, we can also probably add UMA to this to see how often "docked" is
> passed in as a parameter, and use that data to determine if it is safe to
remove
> it entirely.

Done. and done. Please take a look.

For the UMA part, I prefer to do this in a later CL.

[commit-bot: I haz the power, 2017-03-17 23:35:22]

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-03-18 01:15:17]

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

[commit-bot: I haz the power, 2017-03-18 01:15:19]

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

[afakhry, 2017-03-21 02:08:57]

Devlin, friendly ping. Thanks!

[Devlin, 2017-03-21 17:34:52]

afakhry wrote:
> For the UMA part, I prefer to do this in a later CL.

That's fine.  Can you file a bug to add UMA and remove the docked enum if
possible?

Otherwise, lgtm.  Thanks for updating the windows.json. :)

[afakhry, 2017-03-21 18:09:12]

On 2017/03/21 17:34:52, Devlin wrote:
> afakhry wrote:
> > For the UMA part, I prefer to do this in a later CL.
> 
> That's fine.  Can you file a bug to add UMA and remove the docked enum if
> possible?
> 
> Otherwise, lgtm.  Thanks for updating the windows.json. :)

Done. I filed crbug.com/703733 to track this. Thanks! :)

[afakhry, 2017-03-21 18:15:08]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-03-21 18:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
oshima@chromium.org, sky@chromium.org, dcheng@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2700523004/#ps330001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-21 18:15:51]

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-03-21 19:27:50]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1490120108895150,
"parent_rev": "ef9521efd71c8e81d462f58a6875f0ff4d1eea55", "commit_rev":
"cd7193637823047ec88226019bb1d987215a35db"}

[commit-bot: I haz the power, 2017-03-21 19:28:58]

Description was changed from

==========
Remove docked windows entirely in M59.

After we hid this feature behind a flag in M57, now it's time to remove it
completely.
This CL removes the feature, its tests, and the newly added flag.

BUG=668355
==========

to

==========
Remove docked windows entirely in M59.

After we hid this feature behind a flag in M57, now it's time to remove it
completely.
This CL removes the feature, its tests, and the newly added flag.

BUG=668355

Review-Url: https://codereview.chromium.org/2700523004
Cr-Commit-Position: refs/heads/master@{#458508}
Committed:
https://chromium.googlesource.com/chromium/src/+/cd7193637823047ec88226019bb1...
==========

[commit-bot: I haz the power, 2017-03-21 19:29:01]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/cd7193637823047ec88226019bb1...

[sadrul, 2017-04-13 17:00:21]

On 2017/03/10 01:18:29, sadrul wrote:
> drive-by comment
> 
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> File ash/public/cpp/shell_window_ids.h (right):
> 
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> ash/public/cpp/shell_window_ids.h:113: const int32_t kAllShellContainerIds[] =
{
> With the introduction of enums, this will go away, right? Instead of adding
this
> with this CL, and removing it later, would it be possible to convert to enums
> first, and then land this CL? (that would also make it easier to do a revert
if
> needed for some reason)

The CL has landed a while back. Can you please fix this now?

[afakhry, 2017-04-13 19:55:55]

On 2017/04/13 17:00:21, sadrul wrote:
> On 2017/03/10 01:18:29, sadrul wrote:
> > drive-by comment
> > 
> >
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> > File ash/public/cpp/shell_window_ids.h (right):
> > 
> >
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> > ash/public/cpp/shell_window_ids.h:113: const int32_t kAllShellContainerIds[]
=
> {
> > With the introduction of enums, this will go away, right? Instead of adding
> this
> > with this CL, and removing it later, would it be possible to convert to
enums
> > first, and then land this CL? (that would also make it easier to do a revert
> if
> > needed for some reason)
> 
> The CL has landed a while back. Can you please fix this now?

I have a CL for this: https://codereview.chromium.org/2751523003/ but I need to
rebase it. I will do this very soon. Thanks for the reminder!

[afakhry, 2017-04-13 22:53:07]

On 2017/04/13 19:55:55, afakhry wrote:
> On 2017/04/13 17:00:21, sadrul wrote:
> > On 2017/03/10 01:18:29, sadrul wrote:
> > > drive-by comment
> > > 
> > >
> >
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> > > File ash/public/cpp/shell_window_ids.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2700523004/diff/220001/ash/public/cpp/shell_w...
> > > ash/public/cpp/shell_window_ids.h:113: const int32_t
kAllShellContainerIds[]
> =
> > {
> > > With the introduction of enums, this will go away, right? Instead of
adding
> > this
> > > with this CL, and removing it later, would it be possible to convert to
> enums
> > > first, and then land this CL? (that would also make it easier to do a
revert
> > if
> > > needed for some reason)
> > 
> > The CL has landed a while back. Can you please fix this now?
> 
> I have a CL for this: https://codereview.chromium.org/2751523003/ but I need
to
> rebase it. I will do this very soon. Thanks for the reminder!

Done. Doing a dry-run to make sure everything is ok, then I'll have it reviewed.