Quick fix to get 7DA counts for unified Chrome / App Launcher.

Quick fix to get 7DA counts for unified Chrome / App Launcher.

To enable 7DA counts for App Launcher, the following is needed:

- On execution, App Launcher sets "dr" to "1" in its DidRun key, i.e.:
HKCU\Software\Google\Update\ClientState\{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}\dr

- "Present version" key is set for the App Launcher's clients (at user/system level):
(HKCU\HKLM)\Software\Google\Update\Clients\{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}\pv

- Google Update is installed.

Ultimately we wish to rely on https://chromiumcodereview.appspot.com/14031025 to perform the proper migration.  However, there is need to get the 7DA data ASAP, so this CL implements a quick fix to set the "dr" key. Specifically:

- In app_list_controller_win.cc: Removed check for presence of obsolete App Launcher artifacts, so the "dr" key (in user-level) is written whenever App Launcher runs.
- In install_worker.cc: If multi-install Chrome is installed, *in the absence* of the deprecated App Launcher (needed for backward compatiblity), create "shadow" App Launcher key for "pv".
- In uninstall.cc: Cleanup this shadow App Launcher key. Again, backward compatibility is support people who have the old stand-alone App Launcher, before we land the real fix 14031025.

BUG=233295
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199123

[huangs, 2013-05-07 20:24:57]

Previously I thought this would be too much trouble, but I learned from grt@
that what we really need is to update the "pv" key, not the "ap" key. This
enables the present quick fix. PTAL.

[grt (UTC plus 2), 2013-05-07 20:52:02]

https://codereview.chromium.org/14711006/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:160: false /*
user-level */);
"launcher_state == chrome_launcher_support::INSTALLED_AT_SYSTEM_LEVEL" was
correct. Technically speaking, the code in install_worker.cc should also write
UninstallArguments to the ClientState key with --multi-install so that
InstallUtil::IsMultiInstall returns true for this distribution.

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:1230: if
(installer_state.is_multi_install() &&
Please add to the comment that this magically omits the new version key work
items for Chrome canary by way of the is_multi_install(). I imagine that's what
you want (correct me if I'm wrong).

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1238: if
(original_state.GetProductState(false,
I think you want simply:
    if (installer_state.is_multi_install()) {
      ...
    }
so that this happens for all multi- Chrome browser uninstalls. This prevents
uninstallation of Chrome canary from erroneously deleting the app host's version
key.

[grt (UTC plus 2), 2013-05-07 20:57:30]

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1238: if
(original_state.GetProductState(false,
On 2013/05/07 20:52:02, grt wrote:
> I think you want simply:
>     if (installer_state.is_multi_install()) {
>       ...
>     }
> so that this happens for all multi- Chrome browser uninstalls. This prevents
> uninstallation of Chrome canary from erroneously deleting the app host's
version
> key.

As discussed in person, this suggesting isn't quite right. I'd forgotten about
the case where the user has the old-style app launcher installed and only Chrome
is being uninstalled.

[huangs, 2013-05-08 01:16:53]

My main uncertainty is, right now I only limit the feature to multi-install
Chrome.  Therefore non-multi-install Chrome and Chrome Canary will *not* get 7DA
user tracking.

benwells@: Could you verify this?  I'm going to upload a patch with the
restriction, but we can always remove it.

[huangs, 2013-05-08 01:22:02]

Basic goals:
- After install, "shadow" App Launcher should/should not appear.
- After running "chrome.exe --show-app-list", user-level App Launcher "dr"
should/should not be set.

Applied to the following cases:

- {User-level, system-level} multi-install Chrome ==> has shadow, has "dr".
- {User-level, system-level} stand-alone Chrome ==> no shadow, no "dr".
- User-level Chrome Canary ==> no shadow, *** no "dr" ***

After uninstall, no shadow should linger (although "dr" lingers, presumably by
design, since Chrome does it, too).

Compatibility with legacy App Launcher (27.0.1444.3), to avoid Frankensteins:

- Install legacy App Launcher, update ==> no shadow (registry already exists),
has "dr".  
- Install legacy App Launcher, install new Chrome ==> no shadow (registry
already exists), has "dr".  
  - Uninstall Chrome ==> old registry still exists, old App Launcher artifacts
exists; "dr" still works.

PTAL.

https://codereview.chromium.org/14711006/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:160: false /*
user-level */);
Per discussion, going to pass whether or not the current chrome.exe is on
user-level or system-level. To this end, using the same idea as:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:1230: if
(installer_state.is_multi_install() &&
Done.  I assume that Chrome Canary does not have 7DA tracking; so the same
should hold for App Launcher.

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1238: if
(original_state.GetProductState(false,
Changed how existing App Launcher is detected; updated comment.

https://codereview.chromium.org/14711006/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1238: if
(original_state.GetProductState(false,
Also added check for multi-install, to prevent useless "dr" flags being set for
Canary and non-multi-install Chrome.

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:162:
BrowserDistribution* chrome_binaries_dist =
Need to use |chrome_binaries_dist|, so IsMultiInstall() grabs the right data
from the registry.  Note that Chrome registry is absent for legacy stand-alone
App Launchers; and "shadow" App Launchers are missing the "ap" values for the
check.

If we want tracking for non-multi-install, this can be significantly simplified.

[benwells, 2013-05-08 02:09:34]

I'm gonna go ahead and rubber stamp lgtm the changes to app_list_controller_win.

grt: can you look at the changes there and make sure they are sane?

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:162:
BrowserDistribution* chrome_binaries_dist =
On 2013/05/08 01:22:02, huangs1 wrote:
> Need to use |chrome_binaries_dist|, so IsMultiInstall() grabs the right data
> from the registry.  Note that Chrome registry is absent for legacy stand-alone
> App Launchers; and "shadow" App Launchers are missing the "ap" values for the
> check.
> 
> If we want tracking for non-multi-install, this can be significantly
simplified.

We only need tracking for normal chrome users. Does that help answer your
question?

[huangs, 2013-05-08 02:47:40]

Thanks! If by normal you mean {Stable, Beta, Dev. channel}, then this should be
fine.

[benwells, 2013-05-08 04:40:38]

On 2013/05/08 02:47:40, huangs1 wrote:
> Thanks! If by normal you mean {Stable, Beta, Dev. channel}, then this should
be
> fine.

That's what I meant. I don't think it matters if we include canary or not, you
could argue it either way.

For my knowledge: does canary not use multi install?

[grt (UTC plus 2), 2013-05-08 19:36:09]

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/14711006/diff/16001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:162:
BrowserDistribution* chrome_binaries_dist =
As discussed, please add a comment here stating that the binaries' dist is used
since Chrome browser may not be installed for the legacy case. Also mention that
if Canary and regular Chrome are installed, this will mark the regular Chrome's
app host as having been run (and file a bug).

https://codereview.chromium.org/14711006/diff/16001/chrome/installer/setup/un...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/14711006/diff/16001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1237: // Remove the "shadow" App Launcher
registry keys that gat added.
nit: remove " that gat added"

https://codereview.chromium.org/14711006/diff/16001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1244: bool uninstalling_app_launcher =
product.is_chrome_app_host();
this will always be false since this whole code block is within an (is_chrome)
conditional.

[huangs, 2013-05-08 20:17:59]

Filed http://crbug.com/239163 for the edge case.
PTAL.

https://chromiumcodereview.appspot.com/14711006/diff/16001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/14711006/diff/16001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:1237: // Remove the "shadow" App Launcher
registry keys that gat added.
On 2013/05/08 19:36:09, grt wrote:
> nit: remove " that gat added"

Done.

https://chromiumcodereview.appspot.com/14711006/diff/16001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:1244: bool uninstalling_app_launcher =
product.is_chrome_app_host();
This was meant for the edge case of simultaneous uninstall of legacy App
Launcher (that got updated) and Chrome: we should let App Launcher uninstaller
take care of itself.  However, a normal user would not uninstall two things at
the same time, so removed code.

[grt (UTC plus 2), 2013-05-08 20:31:04]

https://chromiumcodereview.appspot.com/14711006/diff/29001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/14711006/diff/29001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:1241: bool has_legacy_app_launcher =
make this true only if the app launcher is installed at the same level as the
current operation

[huangs, 2013-05-08 20:38:57]

PTAL.

https://chromiumcodereview.appspot.com/14711006/diff/29001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/14711006/diff/29001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:1241: bool has_legacy_app_launcher =
On 2013/05/08 20:31:04, grt wrote:
> make this true only if the app launcher is installed at the same level as the
> current operation

Done.

[grt (UTC plus 2), 2013-05-08 20:46:18]

lgtm

[commit-bot: I haz the power, 2013-05-08 22:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/14711006/38001

[commit-bot: I haz the power, 2013-05-09 05:30:10]

Change committed as 199123