Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org

Committed: https://crrev.com/38d242d4d7da0d7cfbf0841b53f57803f7f1156c
Cr-Commit-Position: refs/heads/master@{#369527}

[gab, 2016-01-14 16:28:31]

Description was changed from

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
==========

to

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
==========

[gab, 2016-01-14 16:30:27]

Patchset #1 (id:1) has been deleted

[gab, 2016-01-14 16:32:52]

Description was changed from

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
==========

to

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
==========

[gab, 2016-01-14 16:33:13]

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

[gab, 2016-01-14 16:33:13]

Greg PTAL, thanks!

[grt (UTC plus 2), 2016-01-14 16:40:51]

lgtm w/ a few comment nits. nice!

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:163: // have the appropriate AppUserModelId.
Also clear the dual_mode property
clear -> clears

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:164: // from shortcuts that previously had as
requested by |clear_dual_mode|.
had as -> had it if

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win_unittest.cc (right):

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win_unittest.cc:313: // Test migration when
also clearing for dual mode.
nix "for"

[gab, 2016-01-14 17:09:29]

Thanks, done.

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:163: // have the appropriate AppUserModelId.
Also clear the dual_mode property
On 2016/01/14 16:40:51, grt wrote:
> clear -> clears

Done.

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:164: // from shortcuts that previously had as
requested by |clear_dual_mode|.
On 2016/01/14 16:40:51, grt wrote:
> had as -> had it if

Done.

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win_unittest.cc (right):

https://codereview.chromium.org/1586143002/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win_unittest.cc:313: // Test migration when
also clearing for dual mode.
On 2016/01/14 16:40:51, grt wrote:
> nix "for"

Done.

[gab, 2016-01-14 17:09:33]

The CQ bit was checked by gab@chromium.org

[gab, 2016-01-14 17:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1586143002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2016-01-14 17:09:48]

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

[commit-bot: I haz the power, 2016-01-14 17:09:49]

This CL has an open dependency (Issue 1573243010 Patch 20001). Please resolve
the dependency and try again.

[gab, 2016-01-14 17:14:41]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-01-14 17:16:13]

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

[gab, 2016-01-14 17:16:13]

TBR sky for shell_integration.h (#ifdef(OS_WIN) only change and OWNER approval
already in for the _win.cc side) and related TODO in
startup_browser_creator_impl.cc

[gab, 2016-01-14 17:16:26]

Description was changed from

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
==========

to

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org
==========

[scottmg, 2016-01-14 18:14:03]

Thanks Gab.

[gab, 2016-01-14 19:36:49]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-01-14 19:38:52]

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

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

Description was changed from

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org
==========

to

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-01-14 20:38:21]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-14 20:39:28]

Description was changed from

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org
==========

to

==========
Cleanup dual_mode handling in ShellIntegrationWin after Metro removal.

Follow-up to https://codereview.chromium.org/1581473002

Per MSDN documentation it appears that we have no choice but to set the
dual_mode property to false when it is already set: "Removing a property
values from a property store is not supported and could lead to unexpected
results." (though this comes right after the documentation saying removal is
one of the three use cases of IPropertyStore::SetValue()..!)
https://msdn.microsoft.com/en-us/library/windows/desktop/bb761475(v=vs.85).aspx

Can be cleaned up further as a follow-up by getting rid of the dual_mode
field on ShortcutProperties altogether but that requires more thought.

Also, adding TODO for self to remove shortcut migration from the startup
path now that we have a mean to do this in setup for per-user shortcuts
via Active Setup.

BUG=558054, 577697
TBR=sky@chromium.org

Committed: https://crrev.com/38d242d4d7da0d7cfbf0841b53f57803f7f1156c
Cr-Commit-Position: refs/heads/master@{#369527}
==========

[commit-bot: I haz the power, 2016-01-14 20:39:28]

Patchset 2 (id:??) landed as
https://crrev.com/38d242d4d7da0d7cfbf0841b53f57803f7f1156c
Cr-Commit-Position: refs/heads/master@{#369527}