Consolidate two IsArcWindow functions to reduce code duplication

Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try

Review-Url: https://codereview.chromium.org/2877883002
Cr-Commit-Position: refs/heads/master@{#472314}
Committed: https://chromium.googlesource.com/chromium/src/+/5f38c3cd32e96ccbf3f37653aeb30e8d268e9d36

[Yusuke Sato, 2017-05-12 00:00:04]

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

[commit-bot: I haz the power, 2017-05-12 00:01:27]

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-05-12 00:19:42]

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

[commit-bot: I haz the power, 2017-05-12 00:19:43]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Yusuke Sato, 2017-05-12 00:30:59]

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

[commit-bot: I haz the power, 2017-05-12 00:32:27]

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-05-12 00:42:17]

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

[commit-bot: I haz the power, 2017-05-12 00:42:18]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuke Sato, 2017-05-12 01:29:34]

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

[Yusuke Sato, 2017-05-12 01:30:22]

Description was changed from

==========
wip: Consolidate two IsArcWindow functions to reduce code duplication

BUG=None
TEST=try
==========

to

==========
wip: Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better code coverage.

BUG=None
TEST=try
==========

[commit-bot: I haz the power, 2017-05-12 01:30:36]

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-05-12 01:42:12]

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

[commit-bot: I haz the power, 2017-05-12 01:42:13]

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

[Yusuke Sato, 2017-05-12 02:01:56]

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

[commit-bot: I haz the power, 2017-05-12 02:03:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-12 02:04:23]

Patchset #2 (id:20001) has been deleted

[Yusuke Sato, 2017-05-12 02:04:31]

Patchset #2 (id:40001) has been deleted

[Yusuke Sato, 2017-05-12 02:32:20]

Description was changed from

==========
wip: Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better code coverage.

BUG=None
TEST=try
==========

to

==========
wip: Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try
==========

[Yusuke Sato, 2017-05-12 03:14:38]

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

[commit-bot: I haz the power, 2017-05-12 03:15:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-12 03:54:45]

Patchset #2 (id:60001) has been deleted

[Yusuke Sato, 2017-05-12 04:00:41]

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

[commit-bot: I haz the power, 2017-05-12 04:01:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-12 04:10:51]

Patchset #2 (id:80001) has been deleted

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

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

[commit-bot: I haz the power, 2017-05-12 05:16:55]

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

[Yusuke Sato, 2017-05-12 20:11:00]

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

[Yusuke Sato, 2017-05-12 20:11:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-12 20:11:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-12 20:12:19]

Description was changed from

==========
wip: Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try
==========

to

==========
Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try
==========

[Yusuke Sato, 2017-05-12 20:14:57]

yusukes@chromium.org changed reviewers:
+ cylee@chromium.org, lhchavez@chromium.org, oshima@chromium.org

[Yusuke Sato, 2017-05-12 20:14:58]

OWNERs, please have a look.

Luis: **/arc/
Cheng-Yu: c/b/memory/
Oshima: components/arc/DEPS and components/arc/arc_util.cc

Thanks!

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (left):

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:78: std::string
application_id = exo::ShellSurface::GetApplicationId(window);
oshima: arc_util.cc's implementation doesn't do this string comparison, and
instead, checks if the window's AppType is ash::AppType::ARC_APP. I believe
arc_util.cc's way doesn't have any disadvantages (and is slightly
faster/simpler) but please let me know if I'm wrong.

[Luis Héctor Chávez, 2017-05-12 20:44:19]

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (left):

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:41: #include
"components/exo/shell_surface.h"
Can this also be removed from the unittest?

https://codereview.chromium.org/2877883002/diff/120001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2877883002/diff/120001/components/arc/arc_uti...
components/arc/arc_util.h:73: // Returns true if |window| is associated with an
ash::WmWindow whose type is
nit: s/ash::WmWindow/aura::Window/

[cylee1, 2017-05-12 20:57:27]

lgtm tab_manager_delegate_chromeos.cc

[Yusuke Sato, 2017-05-12 21:05:52]

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

[Yusuke Sato, 2017-05-12 21:06:13]

Thanks, PTAL

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (left):

https://codereview.chromium.org/2877883002/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:41: #include
"components/exo/shell_surface.h"
On 2017/05/12 20:44:19, Luis Héctor Chávez wrote:
> Can this also be removed from the unittest?

Good catch! Done (with many other #includes)

https://codereview.chromium.org/2877883002/diff/120001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2877883002/diff/120001/components/arc/arc_uti...
components/arc/arc_util.h:73: // Returns true if |window| is associated with an
ash::WmWindow whose type is
On 2017/05/12 20:44:19, Luis Héctor Chávez wrote:
> nit: s/ash::WmWindow/aura::Window/

Rephrased the comment. I think the original one was okay (although it was
probably too detailed.)

[commit-bot: I haz the power, 2017-05-12 21:07:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Luis Héctor Chávez, 2017-05-12 21:27:54]

lgtm defer to oshima.

[commit-bot: I haz the power, 2017-05-12 23:16:21]

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

[commit-bot: I haz the power, 2017-05-12 23:16:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[oshima, 2017-05-13 01:06:09]

https://codereview.chromium.org/2877883002/diff/140001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2877883002/diff/140001/components/arc/arc_uti...
components/arc/arc_util.cc:121: return wm_window->GetAppType() ==
static_cast<int>(ash::AppType::ARC_APP);
Could you please use GetProperty(aura::client::kAppType) instead and change the
dependency to ash/shared.

[Yusuke Sato, 2017-05-15 20:51:57]

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

[commit-bot: I haz the power, 2017-05-15 20:53:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-15 20:54:20]

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

[commit-bot: I haz the power, 2017-05-15 20:55:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-15 21:13:27]

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

[Yusuke Sato, 2017-05-15 21:13:52]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-05-15 21:14:16]

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-05-15 21:37:31]

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

[commit-bot: I haz the power, 2017-05-15 21:37:32]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuke Sato, 2017-05-15 21:52:24]

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

[Yusuke Sato, 2017-05-15 21:52:45]

Patchset #5 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-05-15 21:53:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-15 21:57:50]

Thanks, please take another look.

https://codereview.chromium.org/2877883002/diff/140001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2877883002/diff/140001/components/arc/arc_uti...
components/arc/arc_util.cc:121: return wm_window->GetAppType() ==
static_cast<int>(ash::AppType::ARC_APP);
On 2017/05/13 01:06:09, oshima wrote:
> Could you please use GetProperty(aura::client::kAppType) instead and change
the
> dependency to ash/shared.

Done.

[commit-bot: I haz the power, 2017-05-15 23:11:38]

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

[commit-bot: I haz the power, 2017-05-15 23:11:39]

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

[oshima, 2017-05-16 00:39:26]

lgtm

[Yusuke Sato, 2017-05-16 01:13:27]

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

[Yusuke Sato, 2017-05-16 01:13:28]

+sadrul
Could you do an owner review? I'm adding ui/aura/ to components/arc/DEPS.

[sadrul, 2017-05-16 18:21:51]

lgtm

https://codereview.chromium.org/2877883002/diff/220001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2877883002/diff/220001/components/arc/BUILD.g...
components/arc/BUILD.gn:124: "//ui/aura",
I don't really know what the difference is between arc_base and arc targets. But
do you want to grow a dependency on ash and aura from something called
"arc_base"? Would it make sense to move this into "arc" instead?

[Yusuke Sato, 2017-05-16 20:33:44]

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

[commit-bot: I haz the power, 2017-05-16 20:35:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-16 21:57:58]

https://codereview.chromium.org/2877883002/diff/220001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2877883002/diff/220001/components/arc/BUILD.g...
components/arc/BUILD.gn:124: "//ui/aura",
On 2017/05/16 18:21:51, sadrul wrote:
> I don't really know what the difference is between arc_base and arc targets.
But
> do you want to grow a dependency on ash and aura from something called
> "arc_base"? Would it make sense to move this into "arc" instead?

Thanks. Chatted with lhchavez@ and moved them to "arc".

[commit-bot: I haz the power, 2017-05-17 00:10:28]

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

[commit-bot: I haz the power, 2017-05-17 00:10:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yusuke Sato, 2017-05-17 00:26:11]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2017-05-17 00:26:12]

The patchset sent to the CQ was uploaded after l-g-t-m from cylee@chromium.org,
lhchavez@chromium.org, oshima@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2877883002/#ps240001
(title: "Address a comment from sadrul@")

[commit-bot: I haz the power, 2017-05-17 00:27:36]

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-05-17 03:56:58]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494980771962430,
"parent_rev": "3edd0dad3fd5511ded2a49ce0ce2642718f8801c", "commit_rev":
"5f38c3cd32e96ccbf3f37653aeb30e8d268e9d36"}

[commit-bot: I haz the power, 2017-05-17 03:57:14]

Description was changed from

==========
Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try
==========

to

==========
Consolidate two IsArcWindow functions to reduce code duplication

Also add a unittest for better test coverage.

BUG=None
TEST=try

Review-Url: https://codereview.chromium.org/2877883002
Cr-Commit-Position: refs/heads/master@{#472314}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f38c3cd32e96ccbf3f37653aeb3...
==========

[commit-bot: I haz the power, 2017-05-17 03:57:15]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/5f38c3cd32e96ccbf3f37653aeb3...