Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902963002:
Add a new LaunchMode value for launches from a user experiment. (Closed)
|
Created:
3 years, 7 months ago by grt (UTC plus 2) Modified:
3 years, 6 months ago CC:
chromium-reviews, skare_ Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a new LaunchMode value for launches from a user experiment.
This CL also moves the Launch.Modes histogram and its enum into the
Chromium repo.
BUG=717091
R=pkasting@chromium.org
Review-Url: https://codereview.chromium.org/2902963002
Cr-Commit-Position: refs/heads/master@{#475846}
Committed: https://chromium.googlesource.com/chromium/src/+/3e11266cc846a7ec3abf54c5e7c25e958caa5eef
Patch Set 1 #Patch Set 2 : with histogram move #
Total comments: 11
Patch Set 3 : pk feedback #
Total comments: 2
Patch Set 4 : LM_OTHER #
Messages
Total messages: 26 (16 generated)
Description was changed from ========== Add a new LaunchMode value for launches from a user experiment. BUG=717091 R=pkasting@chromium.org ========== to ========== Add a new LaunchMode value for launches from a user experiment. BUG=717091 R=pkasting@chromium.org ==========
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
PTAL. The corresponding enum for the backend is in the internal histograms.xml. I have a separate Cl out to update it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add a new LaunchMode value for launches from a user experiment. BUG=717091 R=pkasting@chromium.org ========== to ========== Add a new LaunchMode value for launches from a user experiment. This CL also moves the Launch.Modes histogram and its enum into the Chromium repo. BUG=717091 R=pkasting@chromium.org ==========
grt@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for metrics changes. Thanks.
lgtm
Note: I'm behind on codereviews but will try and get to this today (Thursday May 25), after I get some sleep.
LGTM https://codereview.chromium.org/2902963002/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2902963002/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:852: const char kUserExperimentUx[] = "user-experiment-ux"; I'm not terribly keen on a switch called "user experiment UX" because that sounds like it applies to practically every UX change we make. Can we talk about toasts, or launching, or something more descriptive in this name? Based on the comment, "show-try-chrome-dialog"? Or, based on the enum description in the next file, "show-installer-retention-ui"? (And a similar rename of the variable.) Nit: Indent the initialization part even with the lines above and below https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20826: + <int value="3" label="LM_SHORTCUT_NONE">Not launched from a shortcut</int> Is this mutually exclusive with the other items that aren't LM_SHORTCUT_XXX? Because the description here doesn't really imply that. https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20828: + Launched from shortcut but no name available "Name" in what sense? https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20834: + Launched from the quick launch bar (pre-Win7) Seems like this value must be deprecated since we don't support Win 7 anymore. https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25757: + <summary>The different ways chrome is launched.</summary> Nit: Capitalize Chrome?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, PTAL. https://codereview.chromium.org/2902963002/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2902963002/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:852: const char kUserExperimentUx[] = "user-experiment-ux"; On 2017/05/26 00:57:41, Peter Kasting wrote: > I'm not terribly keen on a switch called "user experiment UX" because that > sounds like it applies to practically every UX change we make. > > Can we talk about toasts, or launching, or something more descriptive in this > name? Based on the comment, "show-try-chrome-dialog"? We may as well re-use the existing --try-chrome-again switch. Thanks. https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20826: + <int value="3" label="LM_SHORTCUT_NONE">Not launched from a shortcut</int> On 2017/05/26 00:57:41, Peter Kasting wrote: > Is this mutually exclusive with the other items that aren't LM_SHORTCUT_XXX? > Because the description here doesn't really imply that. Done. https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20828: + Launched from shortcut but no name available On 2017/05/26 00:57:41, Peter Kasting wrote: > "Name" in what sense? Done. https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20834: + Launched from the quick launch bar (pre-Win7) On 2017/05/26 00:57:42, Peter Kasting wrote: > Seems like this value must be deprecated since we don't support Win 7 anymore. We still create the QL shortcut for all installs and still have data arriving from XP/Vista installs. It's difficult but not impossible to use the QL shortcut on Win7+ machines if one were to follow one of the online guides for resurrecting the QL area of the taskbar. I'm inclined to leave this as-is until such time as the code is removed. WDYT? https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25757: + <summary>The different ways chrome is launched.</summary> On 2017/05/26 00:57:42, Peter Kasting wrote: > Nit: Capitalize Chrome? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2902963002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20834: + Launched from the quick launch bar (pre-Win7) On 2017/05/29 07:08:42, grt (UTC plus 2) wrote: > On 2017/05/26 00:57:42, Peter Kasting wrote: > > Seems like this value must be deprecated since we don't support Win 7 anymore. > > We still create the QL shortcut for all installs and still have data arriving > from XP/Vista installs. It's difficult but not impossible to use the QL shortcut > on Win7+ machines if one were to follow one of the online guides for > resurrecting the QL area of the taskbar. I'm inclined to leave this as-is until > such time as the code is removed. WDYT? OK. https://codereview.chromium.org/2902963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2902963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20827: + Fallback value when no other LM_ condition is satisfied Should this be LM_OTHER (here and in the code), then?
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2902963002/#ps80001 (title: "LM_OTHER")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2902963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2902963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:20827: + Fallback value when no other LM_ condition is satisfied On 2017/05/31 01:54:02, Peter Kasting wrote: > Should this be LM_OTHER (here and in the code), then? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496214212031310,
"parent_rev": "65b46abb94eacb1bfd79113badafb26554a93705", "commit_rev":
"3e11266cc846a7ec3abf54c5e7c25e958caa5eef"}
Message was sent while issue was closed.
Description was changed from ========== Add a new LaunchMode value for launches from a user experiment. This CL also moves the Launch.Modes histogram and its enum into the Chromium repo. BUG=717091 R=pkasting@chromium.org ========== to ========== Add a new LaunchMode value for launches from a user experiment. This CL also moves the Launch.Modes histogram and its enum into the Chromium repo. BUG=717091 R=pkasting@chromium.org Review-Url: https://codereview.chromium.org/2902963002 Cr-Commit-Position: refs/heads/master@{#475846} Committed: https://chromium.googlesource.com/chromium/src/+/3e11266cc846a7ec3abf54c5e7c2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3e11266cc846a7ec3abf54c5e7c2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
