Renames WmShell to ShellPort

Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org, stevenjb@chromium.org
TBR=elijahtaylor

Review-Url: https://codereview.chromium.org/2808723004
Cr-Commit-Position: refs/heads/master@{#463444}
Committed: https://chromium.googlesource.com/chromium/src/+/11cf8db913c11ee87ed08bdd853427fc7c23fe67

[sky, 2017-04-10 20:38:08]

Description was changed from

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org
==========

to

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
stevenjb@chromium.org
==========

[sky, 2017-04-10 20:38:08]

sky@chromium.org changed reviewers:
+ reveman@chromium.org, stevenjb@chromium.org

[sky, 2017-04-10 20:38:37]

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

[sky, 2017-04-10 20:41:11]

elijahtaylor: arc
reveman: exo
stevenjb: chrome/browser/ui/webui/options/browser_options_handler.cc (sorry for
the big change to this file, blame git cl format)
jamescook: the rest

[sky, 2017-04-10 20:41:19]

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

[commit-bot: I haz the power, 2017-04-10 20:42: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-10 20:48:30]

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

[commit-bot: I haz the power, 2017-04-10 20:48:32]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reveman, 2017-04-10 20:50:28]

exo lgtm % nit

https://codereview.chromium.org/2808723004/diff/20001/components/exo/pointer_...
File components/exo/pointer_unittest.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/components/exo/pointer_...
components/exo/pointer_unittest.cc:5: #include "components/exo/pointer.h"
nit: blank line after this

[sky, 2017-04-10 20:58:37]

https://codereview.chromium.org/2808723004/diff/20001/components/exo/pointer_...
File components/exo/pointer_unittest.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/components/exo/pointer_...
components/exo/pointer_unittest.cc:5: #include "components/exo/pointer.h"
On 2017/04/10 20:50:28, reveman wrote:
> nit: blank line after this

THis is a bug in git cl format. I fixed this and filed 710164.

[sky, 2017-04-10 20:59:36]

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

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

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

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

[commit-bot: I haz the power, 2017-04-10 21:07:10]

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-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2017-04-10 21:33:18]

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

[commit-bot: I haz the power, 2017-04-10 21:34:38]

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

[James Cook, 2017-04-10 22:00:41]

LGTM with nits and a couple questions for the future.

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
File ash/aura/shell_port_classic.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:21: class ASH_EXPORT ShellPortClassic : public
ShellPort,
While you're here can you add a brief class comment? Maybe point to the README
for classic vs. mash?

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:33: // WmShell:
nit: ShellPort:

(I wonder if a grep for WmShell would turn up other references.)

https://codereview.chromium.org/2808723004/diff/20001/ash/metrics/pointer_met...
File ash/metrics/pointer_metrics_recorder.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/metrics/pointer_met...
ash/metrics/pointer_metrics_recorder.cc:91:
ash::ShellPort::Get()->AddPointerWatcher(
nit: ash not needed

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:5: #ifndef ASH_MUS_BRIDGE_SHELL_PORT_MASH_H_
Not necessarily for this CL: Should ash/mus/bridge/* move into /ash/mus?

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:30: namespace mus {
Not for this CL, do you intend to maintain a separate ash::mus namespace? I
don't think it's a useful distinction anymore.

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:38: // WmShell implementation for mus.
nit: ShellPort.

optional: Maybe refer to README also?

https://codereview.chromium.org/2808723004/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:316: (ShellPort::Get()->IsRunningInMash() ||
Shell::HasInstance()));
nit: ShellPort::HasInstance() && Shell::HasInstance() I think. Or maybe just
Shell::HasInstance?

https://codereview.chromium.org/2808723004/diff/20001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator_unittest.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator_unittest.cc:5: #include
"ash/rotator/screen_rotation_animator.h"
nit: blank line below

https://codereview.chromium.org/2808723004/diff/20001/ash/shell_init_params.h
File ash/shell_init_params.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/shell_init_params.h...
ash/shell_init_params.h:28: // Shell takes ownership of |wm_shell|, if null
ShellPortClassic is created.
nit: |shell_port|. Also, is the next line still true?

Not for this CL: It would be nice if this parameter was always required.

[sky, 2017-04-10 22:16:25]

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
File ash/aura/shell_port_classic.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:21: class ASH_EXPORT ShellPortClassic : public
ShellPort,
On 2017/04/10 22:00:41, James Cook wrote:
> While you're here can you add a brief class comment? Maybe point to the README
> for classic vs. mash?

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:33: // WmShell:
On 2017/04/10 22:00:41, James Cook wrote:
> nit: ShellPort:
> 
> (I wonder if a grep for WmShell would turn up other references.)

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/metrics/pointer_met...
File ash/metrics/pointer_metrics_recorder.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/metrics/pointer_met...
ash/metrics/pointer_metrics_recorder.cc:91:
ash::ShellPort::Get()->AddPointerWatcher(
On 2017/04/10 22:00:41, James Cook wrote:
> nit: ash not needed

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:5: #ifndef ASH_MUS_BRIDGE_SHELL_PORT_MASH_H_
On 2017/04/10 22:00:41, James Cook wrote:
> Not necessarily for this CL: Should ash/mus/bridge/* move into /ash/mus?

Agreed

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:30: namespace mus {
On 2017/04/10 22:00:41, James Cook wrote:
> Not for this CL, do you intend to maintain a separate ash::mus namespace? I
> don't think it's a useful distinction anymore.

Agreed. In fact I'm also inclined to remove the ash/mus build file. WDYT?

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:38: // WmShell implementation for mus.
On 2017/04/10 22:00:41, James Cook wrote:
> nit: ShellPort.
> 
> optional: Maybe refer to README also?

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:316: (ShellPort::Get()->IsRunningInMash() ||
Shell::HasInstance()));
On 2017/04/10 22:00:41, James Cook wrote:
> nit: ShellPort::HasInstance() && Shell::HasInstance() I think. Or maybe just
> Shell::HasInstance?

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator_unittest.cc (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator_unittest.cc:5: #include
"ash/rotator/screen_rotation_animator.h"
On 2017/04/10 22:00:41, James Cook wrote:
> nit: blank line below

Done.

https://codereview.chromium.org/2808723004/diff/20001/ash/shell_init_params.h
File ash/shell_init_params.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/shell_init_params.h...
ash/shell_init_params.h:28: // Shell takes ownership of |wm_shell|, if null
ShellPortClassic is created.
On 2017/04/10 22:00:41, James Cook wrote:
> nit: |shell_port|. Also, is the next line still true?

Once ash is the default and the classic code isn't needed it can be removed. I
nuked the comment.

> Not for this CL: It would be nice if this parameter was always required.

Indeed.

[sky, 2017-04-10 22:17:26]

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

[commit-bot: I haz the power, 2017-04-10 22:18:00]

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

[stevenjb, 2017-04-10 23:09:37]

lgtm

https://codereview.chromium.org/2808723004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/2808723004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:255:
{"accessibilityFeaturesLink", IDS_OPTIONS_ACCESSIBILITY_FEATURES_LINK},
Mass reformatting: Ugh. We've been fixing these in smaller pieces as we go...

[James Cook, 2017-04-10 23:12:03]

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.h (right):

https://codereview.chromium.org/2808723004/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:30: namespace mus {
On 2017/04/10 22:16:25, sky wrote:
> On 2017/04/10 22:00:41, James Cook wrote:
> > Not for this CL, do you intend to maintain a separate ash::mus namespace? I
> > don't think it's a useful distinction anymore.
> 
> Agreed. In fact I'm also inclined to remove the ash/mus build file. WDYT?

I'm OK with removing ash/mus/BUILD.gn.

[sky, 2017-04-10 23:20:14]

Description was changed from

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
stevenjb@chromium.org
==========

to

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
stevenjb@chromium.org
TBR=elijahtaylor
==========

[sky, 2017-04-10 23:20:36]

elijahtaylor to TBR for trivial removal of unnecessary includes.

[sky, 2017-04-10 23:20:46]

The CQ bit was unchecked by sky@chromium.org

[sky, 2017-04-10 23:20:56]

The CQ bit was checked by sky@chromium.org

[sky, 2017-04-10 23:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2808723004/#ps80001
(title: "feedback")

[commit-bot: I haz the power, 2017-04-10 23:21:24]

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-10 23:38:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491866456212490,
"parent_rev": "0e704d39cb4a4f6382c87e7e22dbac4e85c57e7c", "commit_rev":
"11cf8db913c11ee87ed08bdd853427fc7c23fe67"}

[commit-bot: I haz the power, 2017-04-10 23:39:17]

Description was changed from

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
stevenjb@chromium.org
TBR=elijahtaylor
==========

to

==========
Renames WmShell to ShellPort

And subclasses as well.

BUG=687662
TEST=none
R=jamescook@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
stevenjb@chromium.org
TBR=elijahtaylor

Review-Url: https://codereview.chromium.org/2808723004
Cr-Commit-Position: refs/heads/master@{#463444}
Committed:
https://chromium.googlesource.com/chromium/src/+/11cf8db913c11ee87ed08bdd8534...
==========

[commit-bot: I haz the power, 2017-04-10 23:39:20]

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