Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14646002:
Add another set of UMA histograms for tab switching on Android. (Closed)
|
Created:
7 years, 7 months ago by Philippe Modified:
7 years, 7 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd another set of UMA histograms for tab switching on Android.
BUG=224278
R=bulach@chromium.org, isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199715
Patch Set 1 : #
Total comments: 3
Patch Set 2 : s/he/the user #
Total comments: 12
Patch Set 3 : Address Ilya's comments #Patch Set 4 : Add new value for TabRestoreUserAction #
Total comments: 2
Patch Set 5 : Address Przemek's comment #Messages
Total messages: 16 (0 generated)
Looks awesome, thanks! One remark below, feel free to skip - I am just thinking aloud. https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:5870: the load or if he gives up by switching to another tab or leaving Chrome. supernit: "if they give up"? Does anybody know if we have a policy on http://en.wikipedia.org/wiki/Singular_they ?
lgtm (but not an owner here), just one small suggestion: https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:5870: the load or if he gives up by switching to another tab or leaving Chrome. On 2013/04/30 12:12:27, ppi wrote: > supernit: "if they give up"? Does anybody know if we have a policy on > http://en.wikipedia.org/wiki/Singular_they ? how about avoiding the issue altogether and name it something like RestoreCompletionStatus? <int value="0" label="RestoredFully"/> <int value="1" label="SwitchedToAnotherTab"/> <int value="2" label="SwitchedToAnotherApp"/> ?
https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:5870: the load or if he gives up by switching to another tab or leaving Chrome. Please note that strictly speaking "RestoredFully" does not exclude "SwitchedToAnotherTab". I think I would prefer to keep the first one as "WaitForCompletion". Another possible easy fix: s/he/the user/. On 2013/04/30 12:38:11, bulach wrote: > On 2013/04/30 12:12:27, ppi wrote: > > supernit: "if they give up"? Does anybody know if we have a policy on > > http://en.wikipedia.org/wiki/Singular_they ? > > how about avoiding the issue altogether and name it something like > RestoreCompletionStatus? > > <int value="0" label="RestoredFully"/> > <int value="1" label="SwitchedToAnotherTab"/> > <int value="2" label="SwitchedToAnotherApp"/> > > ?
On 2013/04/30 12:42:42, ppi wrote: > https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/14646002/diff/5001/tools/metrics/histograms/h... > tools/metrics/histograms/histograms.xml:5870: the load or if he gives up by > switching to another tab or leaving Chrome. > Please note that strictly speaking "RestoredFully" does not exclude > "SwitchedToAnotherTab". I think I would prefer to keep the first one as > "WaitForCompletion". > > Another possible easy fix: s/he/the user/. > > On 2013/04/30 12:38:11, bulach wrote: > > On 2013/04/30 12:12:27, ppi wrote: > > > supernit: "if they give up"? Does anybody know if we have a policy on > > > http://en.wikipedia.org/wiki/Singular_they ? > > > > how about avoiding the issue altogether and name it something like > > RestoreCompletionStatus? > > > > <int value="0" label="RestoredFully"/> > > <int value="1" label="SwitchedToAnotherTab"/> > > <int value="2" label="SwitchedToAnotherApp"/> > > > > ? Thanks guys! I uploaded a patch set replacing 'he' with 'the user'. I did a sync, sorry for the noise when diffing against patch set 1.
https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5902: <histogram name="Tab.RestoreResult" enum="TabRestoreResult"> nit: Perhaps re-use the "BooleanSuccess" enum? https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5909: <histogram name="Tab.RestoreTime" unit="ms"> This should be "units". I'm surprised this wasn't caught by the presubmit check... did you skip that check somehow? https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5910: <summary>Load times for successful tab restores.</summary> Is this per-tab, or for a whole window, when restoring multiple tabs at once? https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9482: <int value="0" label="WaitForCompletion"/> nit: "Wait for completion" https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9483: <int value="1" label="SwitchToAnotherTab"/> nit: "Switch to another tab" https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9484: <int value="2" label="LeaveChrome"/> nit: "Leave Chrome"
Thanks Ilya! https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5902: <histogram name="Tab.RestoreResult" enum="TabRestoreResult"> On 2013/05/01 01:16:05, Ilya Sherman wrote: > nit: Perhaps re-use the "BooleanSuccess" enum? Good point. https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5909: <histogram name="Tab.RestoreTime" unit="ms"> On 2013/05/01 01:16:05, Ilya Sherman wrote: > This should be "units". I'm surprised this wasn't caught by the presubmit > check... did you skip that check somehow? Good catch! The hook was run but the error wasn't propagated correctly, see the output: INFO:root:Loading histograms.xml... INFO:root:Pretty-printing... ERROR:root:Unrecognized attribute unit in tag histogram INFO:root:histograms.xml is correctly pretty-printed. Presubmit checks took 2.2s to calculate. Presubmit checks passed. https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5910: <summary>Load times for successful tab restores.</summary> On 2013/05/01 01:16:05, Ilya Sherman wrote: > Is this per-tab, or for a whole window, when restoring multiple tabs at once? I'm using singular now which should be less confusing. https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9482: <int value="0" label="WaitForCompletion"/> On 2013/05/01 01:16:05, Ilya Sherman wrote: > nit: "Wait for completion" Done. https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9483: <int value="1" label="SwitchToAnotherTab"/> On 2013/05/01 01:16:05, Ilya Sherman wrote: > nit: "Switch to another tab" Done. https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9484: <int value="2" label="LeaveChrome"/> On 2013/05/01 01:16:05, Ilya Sherman wrote: > nit: "Leave Chrome" Done.
On 2013/05/02 08:08:47, Philippe wrote: > Thanks Ilya! > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:5902: <histogram > name="Tab.RestoreResult" enum="TabRestoreResult"> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > nit: Perhaps re-use the "BooleanSuccess" enum? > > Good point. > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:5909: <histogram name="Tab.RestoreTime" > unit="ms"> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > This should be "units". I'm surprised this wasn't caught by the presubmit > > check... did you skip that check somehow? > > Good catch! The hook was run but the error wasn't propagated correctly, see the > output: > INFO:root:Loading histograms.xml... > INFO:root:Pretty-printing... > ERROR:root:Unrecognized attribute unit in tag histogram > INFO:root:histograms.xml is correctly pretty-printed. > > Presubmit checks took 2.2s to calculate. > > Presubmit checks passed. > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:5910: <summary>Load times for successful > tab restores.</summary> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > Is this per-tab, or for a whole window, when restoring multiple tabs at once? > > I'm using singular now which should be less confusing. > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:9482: <int value="0" > label="WaitForCompletion"/> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > nit: "Wait for completion" > > Done. > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:9483: <int value="1" > label="SwitchToAnotherTab"/> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > nit: "Switch to another tab" > > Done. > > https://codereview.chromium.org/14646002/diff/3/tools/metrics/histograms/hist... > tools/metrics/histograms/histograms.xml:9484: <int value="2" > label="LeaveChrome"/> > On 2013/05/01 01:16:05, Ilya Sherman wrote: > > nit: "Leave Chrome" > > Done. Ping :)
LGTM. Sorry, thought I'd LGTM'ed with nits before.
Thanks Philippe! Please find below one suggestion on patch set 4. https://codereview.chromium.org/14646002/diff/16002/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/16002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:9479: <int value="2" label="Close tab"/> It seems that none of these covers the case of the user going to tab switcher and staying there (nice catch!). Maybe we should simplify this to: - Wait for completion - Leave tab (close tab / switch tabs / go to tab switcher) - Leave Chrome ?
Thanks Przemek! https://codereview.chromium.org/14646002/diff/16002/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14646002/diff/16002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:9479: <int value="2" label="Close tab"/> On 2013/05/06 13:34:55, ppi wrote: > It seems that none of these covers the case of the user going to tab switcher > and staying there (nice catch!). > > Maybe we should simplify this to: > - Wait for completion > - Leave tab (close tab / switch tabs / go to tab switcher) > - Leave Chrome > > ? Good idea. I will update the downstream CL accordingly.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/14646002/22001
Retried try job too often on linux_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/14646002/22001
Message was sent while issue was closed.
Committed patchset #5 manually as r199715 (presubmit successful). |
