Remove base/win/metro.{cc|h} and some associated code

Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
Committed: https://crrev.com/969ea45d00215441dd9aa4f5fa2527546e3f5f87
Cr-Commit-Position: refs/heads/master@{#368971}

[scottmg, 2016-01-11 23:13:36]

Description was changed from

==========
Remove base/win/metro.{cc|h} and some associated code

BUG=558054
==========

to

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up affecting a

BUG=558054
==========

[scottmg, 2016-01-11 23:15:41]

Description was changed from

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up affecting a

BUG=558054
==========

to

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

[scottmg, 2016-01-11 23:17:36]

Description was changed from

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

to

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

[scottmg, 2016-01-11 23:17:36]

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

[scottmg, 2016-01-11 23:22:53]

scottmg@chromium.org changed reviewers:
+ ananta@chromium.org, asvitkine@chromium.org, mcasas@chromium.org,
thakis@chromium.org

[scottmg, 2016-01-11 23:22:54]

thakis: base/
sky: chrome/ and ui/
asvitkine: components/metrics/
mcasas: media/capture/video/
ananta: win8/

[Nico, 2016-01-11 23:23:41]

base/ lgtm

[Will Harris, 2016-01-11 23:27:36]

drive-by farewell to all this code.

[sky, 2016-01-12 03:40:51]

Wow, that's a lot of code!
LGTM with the following changes.

Is it possible to get rid of HostDesktopType too (in a separate patch)?

https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
chrome/app/main_dll_loader_win.cc:199: HMODULE dll =
LoadModuleWithDirectory(*module, true);
Looks like this is the only caller of LoadModuleWithDirectory. Nuke the second
param.

https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (left):

https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:152: void
ChromeNativeAppWindowViewsWin::Show() {
Can you remove the override of Show and Activate entirely?

[Alexei Svitkine (slow), 2016-01-12 15:27:25]

components/metrics lgtm

woot!

[mcasas, 2016-01-12 17:25:49]

media/.../win LGTM

https://codereview.chromium.org/1581473002/diff/40001/media/capture/video/win...
File media/capture/video/win/video_capture_device_factory_win.cc (left):

https://codereview.chromium.org/1581473002/diff/40001/media/capture/video/win...
media/capture/video/win/video_capture_device_factory_win.cc:382:
!cmd_line->HasSwitch(switches::kForceDirectShowVideoCapture)) ||
Hmm this flag goes unused, should be removed.
I'll add a bug.

[scottmg, 2016-01-12 18:24:16]

https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
chrome/app/main_dll_loader_win.cc:199: HMODULE dll =
LoadModuleWithDirectory(*module, true);
On 2016/01/12 03:40:51, sky wrote:
> Looks like this is the only caller of LoadModuleWithDirectory. Nuke the second
> param.

Done.

https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (left):

https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:152: void
ChromeNativeAppWindowViewsWin::Show() {
On 2016/01/12 03:40:51, sky wrote:
> Can you remove the override of Show and Activate entirely?

Done.

[scottmg, 2016-01-12 18:25:41]

On 2016/01/12 03:40:51, sky wrote:
> Wow, that's a lot of code!
> LGTM with the following changes.
> 
> Is it possible to get rid of HostDesktopType too (in a separate patch)?

Yeah, it's going to be a surprising amount of code altogether I think. I started
a list here
https://docs.google.com/document/d/108rBCWtpIz7HvbGpj1sqsN5v4R6dWf0wQR_MYvY6o...
. 4-5 of those are in this patch, but definitely HostDesktopType and
SCREEN_TYPE_ALTERNATE will be nice to see the end of.

> 
>
https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
> File chrome/app/main_dll_loader_win.cc (right):
> 
>
https://codereview.chromium.org/1581473002/diff/20001/chrome/app/main_dll_loa...
> chrome/app/main_dll_loader_win.cc:199: HMODULE dll =
> LoadModuleWithDirectory(*module, true);
> Looks like this is the only caller of LoadModuleWithDirectory. Nuke the second
> param.
> 
>
https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc
(left):
> 
>
https://codereview.chromium.org/1581473002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:152: void
> ChromeNativeAppWindowViewsWin::Show() {
> Can you remove the override of Show and Activate entirely?

[scottmg, 2016-01-12 19:03:55]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-01-12 19:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
asvitkine@chromium.org, sky@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1581473002/#ps60001
(title: "fixes")

[commit-bot: I haz the power, 2016-01-12 19:06:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1581473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1581473002/60001

[commit-bot: I haz the power, 2016-01-12 19:18:47]

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

[commit-bot: I haz the power, 2016-01-12 19:18:48]

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

[ananta, 2016-01-12 20:09:15]

metro code lgtm

[scottmg, 2016-01-12 20:10:51]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2016-01-12 20:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1581473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1581473002/60001

[commit-bot: I haz the power, 2016-01-12 20:20:01]

Description was changed from

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

to

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

[commit-bot: I haz the power, 2016-01-12 20:20:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-12 20:20:45]

Description was changed from

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054
==========

to

==========
Remove base/win/metro.{cc|h} and some associated code

The goal was to start by removing base/win/metro.{cc|h}, but this
ended up touching a number of places. So, also removed are:

chrome/browser/metro_utils/
chrome/browser/ui/apps/apps_metro_handler_win.{cc|h}
chrome/browser/ui/metro_pin_tab_helper_win.{cc|h}
chrome/browser/ui/views/frame/browser_frame_ashwin.{cc|h}

In order for the CL to not get even larger, there are some added TODOs
pointing at the associated bug where calls to the above were removed,
but the surrounding code is not yet simplified.

BUG=558054

Committed: https://crrev.com/969ea45d00215441dd9aa4f5fa2527546e3f5f87
Cr-Commit-Position: refs/heads/master@{#368971}
==========

[commit-bot: I haz the power, 2016-01-12 20:20:46]

Patchset 4 (id:??) landed as
https://crrev.com/969ea45d00215441dd9aa4f5fa2527546e3f5f87
Cr-Commit-Position: refs/heads/master@{#368971}

[Will Harris, 2016-01-12 20:20:49]

On 2016/01/12 20:20:02, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001)

[gab, 2016-01-12 21:11:03]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-01-12 21:11:04]

Drive-by on installer stuff (please include me or grt on future installer
changes).

PS: This is awesome!

Thanks!
Gab

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install.cc:399: // mode and that prior to Windows 10
only.
This comment was specific to the removed code and should go as well.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install.cc:401:
start_menu_properties.set_dual_mode(true);
Assuming we can get rid of ShellUtil::ShortcutProperties's dual_mode property
soon if not now.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/i...
chrome/installer/util/install_util.cc:157: base::LaunchOptions launch_options;
inline base::LaunchOptions()

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:302:
entries->back()->set_removal_flag(RegistryEntry::RemovalFlag::VALUE);
This code could be simplified. Only reason to introduce RemovalFlag in the first
place was that this could not simply be removal code but had to be inlined with
insertion code.

It could now simply be unconditional cleanup code and we could get rid of
RemovalFlag which was introduced for this specific use case.

Please add TODOs here as this code looks very weird as such without legacy Metro
context.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:380: // Remove the keys (not only their
values) so that Windows will continue
Ditto here: add TODO

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/u...
File chrome/installer/util/util_constants.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/u...
chrome/installer/util/util_constants.cc:217: const wchar_t kChromeMetroDll[] =
L"metro_driver.dll";
Also need to remove it from FILES.cfg and chrome.release

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:498: if
(reg_key.ReadValueDW(chrome::kLaunchModeValue,
Please add cleanup code to delete this value instead of leaving it rotting
behind.

[scottmg, 2016-01-13 20:48:16]

Thanks Gab, sorry I meant to add one of you for installer but then forgot in the
barrage of adding reviewers as the CL got bigger.

Comments here, but changes in follow up CL here:
https://codereview.chromium.org/1573243010.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install.cc:399: // mode and that prior to Windows 10
only.
On 2016/01/12 21:11:04, gab wrote:
> This comment was specific to the removed code and should go as well.

Done.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install.cc:401:
start_menu_properties.set_dual_mode(true);
On 2016/01/12 21:11:04, gab wrote:
> Assuming we can get rid of ShellUtil::ShortcutProperties's dual_mode property
> soon if not now.

Done.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/i...
chrome/installer/util/install_util.cc:157: base::LaunchOptions launch_options;
On 2016/01/12 21:11:04, gab wrote:
> inline base::LaunchOptions()

Done.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:302:
entries->back()->set_removal_flag(RegistryEntry::RemovalFlag::VALUE);
On 2016/01/12 21:11:04, gab wrote:
> This code could be simplified. Only reason to introduce RemovalFlag in the
first
> place was that this could not simply be removal code but had to be inlined
with
> insertion code.
> 
> It could now simply be unconditional cleanup code and we could get rid of
> RemovalFlag which was introduced for this specific use case.
> 
> Please add TODOs here as this code looks very weird as such without legacy
Metro
> context.

Done. I don't fully understand the code, or when it's used, so just added the
TODO for now.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:380: // Remove the keys (not only their
values) so that Windows will continue
On 2016/01/12 21:11:04, gab wrote:
> Ditto here: add TODO

Done.

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/u...
File chrome/installer/util/util_constants.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/chrome/installer/util/u...
chrome/installer/util/util_constants.cc:217: const wchar_t kChromeMetroDll[] =
L"metro_driver.dll";
On 2016/01/12 21:11:04, gab wrote:
> Also need to remove it from FILES.cfg and chrome.release

Done.

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:498: if
(reg_key.ReadValueDW(chrome::kLaunchModeValue,
On 2016/01/12 21:11:04, gab wrote:
> Please add cleanup code to delete this value instead of leaving it rotting
> behind.

Do we normally do this? How long should we keep the code around for in that
case?

[gab, 2016-01-14 03:10:27]

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:498: if
(reg_key.ReadValueDW(chrome::kLaunchModeValue,
On 2016/01/13 20:48:16, scottmg wrote:
> On 2016/01/12 21:11:04, gab wrote:
> > Please add cleanup code to delete this value instead of leaving it rotting
> > behind.
> 
> Do we normally do this? How long should we keep the code around for in that
> case?

Yes. We do this all over the installer and in chrome prefs. We try not to leave
deprecated state behind. I typically put a TODO to remove the cleanup code in 3
milestones from now (and it typically stays there for a while longer until
somebody notices so >= 6 months on Stable which is plenty for a value that isn't
harmful per se otherwise... we leave cleanup code for > 1 year sometimes for
things that are visible, e.g. deprecated shortcuts)

[gab, 2016-01-14 03:12:24]

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (left):

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:498: if
(reg_key.ReadValueDW(chrome::kLaunchModeValue,
On 2016/01/14 03:10:27, gab wrote:
> On 2016/01/13 20:48:16, scottmg wrote:
> > On 2016/01/12 21:11:04, gab wrote:
> > > Please add cleanup code to delete this value instead of leaving it rotting
> > > behind.
> > 
> > Do we normally do this? How long should we keep the code around for in that
> > case?
> 
> Yes. We do this all over the installer and in chrome prefs. We try not to
leave
> deprecated state behind. I typically put a TODO to remove the cleanup code in
3
> milestones from now (and it typically stays there for a while longer until
> somebody notices so >= 6 months on Stable which is plenty for a value that
isn't
> harmful per se otherwise... we leave cleanup code for > 1 year sometimes for
> things that are visible, e.g. deprecated shortcuts)

Err... >= 4.5 months (18 weeks) on Stable

[gab, 2016-01-14 03:30:37]

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
File win8/delegate_execute/command_execute_impl.cc (right):

https://codereview.chromium.org/1581473002/diff/60001/win8/delegate_execute/c...
win8/delegate_execute/command_execute_impl.cc:456: launch_mode =
ECHUIM_IMMERSIVE;
We should probably get rid of all ECHUIM_IMMERSIVE and AHE_IMMERSIVE options in
delegate_execute, just having them still there while the support backing it is
gone in M49 scares me (since branch cut is imminent and the rest of the removal
won't make it..).

I guess it's all behind switches and keyboard tricks but still...