Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341593003:
Add a user action for stop in Android. (Closed)
DescriptionAdd a user action for stop in Android.
This also splits out reload tracking to be from menu (phones)
vs from toolbar (tablets), which is consistent with the other
actions such as download.
BUG=645294
Committed: https://crrev.com/2f58462eb176193fd017c8a5e724436cb43d37b5
Cr-Commit-Position: refs/heads/master@{#419577}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Clarify actions.xml #Patch Set 3 : update actions.xml forrealz #
Total comments: 3
Patch Set 4 : mpearson@s comment #
Messages
Total messages: 15 (4 generated)
tedchoc@chromium.org changed reviewers: + mpearson@chromium.org
PTAL
https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1590: RecordUserAction.record("MobileToolbarReload"); Ugh, this changes the semantics of MobileToolbarReload. Can you find a new name of the toolbar-only MobileToolbarReload, perhaps MobileToolbarOnlyReload or MobileToolbarRealReload?
https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1590: RecordUserAction.record("MobileToolbarReload"); On 2016/09/15 23:40:24, Mark P wrote: > Ugh, this changes the semantics of MobileToolbarReload. Can you find a new name > of the toolbar-only MobileToolbarReload, perhaps MobileToolbarOnlyReload or > MobileToolbarRealReload? I might be able to be convinced to keep the old user action just with a label describing how its semantics have changed.
I went with updating the actions.xml if I was reading this right. https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2341593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1590: RecordUserAction.record("MobileToolbarReload"); On 2016/09/16 04:19:32, Mark P wrote: > On 2016/09/15 23:40:24, Mark P wrote: > > Ugh, this changes the semantics of MobileToolbarReload. Can you find a new > name > > of the toolbar-only MobileToolbarReload, perhaps MobileToolbarOnlyReload or > > MobileToolbarRealReload? > > I might be able to be convinced to keep the old user action just with a label > describing how its semantics have changed. If I'm reading this correctly, I believe I prefer this. I'd rather split the metrics and just update the descriptions to state the differences pre-m55 to M55+. I only split them because I wanted it to be consistent with the other metrics in terms of naming. If we have to come up with a different naming scheme, then it defeats my goal, so I would just revert my reload changes and add stop.
lgtm with one suggestion below --mark https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9788: MobileToolbarReload summed with MobileMenuReload. the same as MobileToolbarReload -> the same as what MobileToolbarReload should've been
https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9788: MobileToolbarReload summed with MobileMenuReload. On 2016/09/16 22:00:21, Mark P wrote: > the same as MobileToolbarReload > -> > the same as what MobileToolbarReload should've been Color me quite confused. Is this what you are asking? Old: Pre-M55, this was the same as MobileToolbarReload summed with MobileMenuReload. New: Pre-M55, this was the same as what MobileToolbarReload should've been summed with MobileMenuReload. I really have no idea what "should have been" actually means here, or what it clarifies (I'm actually more confused by the sentence with that wording). I'm trying to provide a means of comparison: Pre-M55, MobileToolbarReload accounted for all reload actions. M55+, this metric only refers to reloads that occurred from the toolbar. Or even simpler: Pre-M55, All Reloads = MobileToolbarReload M55+, All Reloads = MobileToolbarReload + MobileMenuReload I could also drop the last sentence, as the "Prior to M55" sentence lets the reader know that something is different, but I was trying to provide clarity around the metric names which simply might not be necessary.
lgtm Sorry to waste so much of your time on this. Please feel free to submit after resolving per the comment below. --mark https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9788: MobileToolbarReload summed with MobileMenuReload. On 2016/09/16 23:40:37, Ted C wrote: > On 2016/09/16 22:00:21, Mark P wrote: > > the same as MobileToolbarReload > > -> > > the same as what MobileToolbarReload should've been > > Color me quite confused. Is this what you are asking? Yes, indeed. I thought the last sentence was confusing. This is the action description for MobileToolbarReload. The last sentence reads: "Pre-M55, this was the same as MobileToolbarReload summed with MobileMenuReload." I had a suggestion on how to clarify it. Apparently my suggestion didn't help. :-( > Old: > Pre-M55, this was the same as MobileToolbarReload summed with MobileMenuReload. > > New: > Pre-M55, this was the same as what MobileToolbarReload should've been summed > with MobileMenuReload. > > I really have no idea what "should have been" actually means here, or what it > clarifies (I'm actually more confused by the sentence with that wording). I'm > trying to provide a means of comparison: > > Pre-M55, MobileToolbarReload accounted for all reload actions. M55+, this > metric only refers to reloads that occurred from the toolbar. > > Or even simpler: > Pre-M55, All Reloads = MobileToolbarReload > M55+, All Reloads = MobileToolbarReload + MobileMenuReload > > I could also drop the last sentence, as the "Prior to M55" sentence lets the > reader know that something is different, but I was trying to provide clarity > around the metric names which simply might not be necessary. Of your three alternate suggestions for clarity, on further rereading I prefer the third--remove the last sentence altogether. The also-acceptable runner-up for me is to replace the last three sentences with your first suggestion: "Pre-M55, MobileToolbarReload accounted for all reload actions. M55+, this metric only refers to reloads that occurred from the toolbar."
On 2016/09/19 18:07:35, Mark P wrote: > lgtm > > Sorry to waste so much of your time on this. Please feel free to submit after > resolving per the comment below. > > --mark > > https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/2341593003/diff/40001/tools/metrics/actions/a... > tools/metrics/actions/actions.xml:9788: MobileToolbarReload summed with > MobileMenuReload. > On 2016/09/16 23:40:37, Ted C wrote: > > On 2016/09/16 22:00:21, Mark P wrote: > > > the same as MobileToolbarReload > > > -> > > > the same as what MobileToolbarReload should've been > > > > Color me quite confused. Is this what you are asking? > > Yes, indeed. > > I thought the last sentence was confusing. This is the action description for > MobileToolbarReload. The last sentence reads: "Pre-M55, this was the same as > MobileToolbarReload summed with MobileMenuReload." > > I had a suggestion on how to clarify it. Apparently my suggestion didn't help. > :-( > > > Old: > > Pre-M55, this was the same as MobileToolbarReload summed with > MobileMenuReload. > > > > New: > > Pre-M55, this was the same as what MobileToolbarReload should've been summed > > with MobileMenuReload. > > > > I really have no idea what "should have been" actually means here, or what it > > clarifies (I'm actually more confused by the sentence with that wording). I'm > > trying to provide a means of comparison: > > > > Pre-M55, MobileToolbarReload accounted for all reload actions. M55+, this > > metric only refers to reloads that occurred from the toolbar. > > > > Or even simpler: > > Pre-M55, All Reloads = MobileToolbarReload > > M55+, All Reloads = MobileToolbarReload + MobileMenuReload > > > > I could also drop the last sentence, as the "Prior to M55" sentence lets the > > reader know that something is different, but I was trying to provide clarity > > around the metric names which simply might not be necessary. > > Of your three alternate suggestions for clarity, on further rereading I prefer > the third--remove the last sentence altogether. > > The also-acceptable runner-up for me is to replace the last three sentences with > your first suggestion: "Pre-M55, MobileToolbarReload accounted for all reload > actions. M55+, this metric only refers to reloads that occurred from the > toolbar." Cool :-), went with the latter as I think it is a slightly more correct. CCTs on tablet use a reload from the menu so calling out phone vs tablet wasn't 100% accurate and dropping that via the runner up seemed best to me.
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2341593003/#ps60001 (title: "mpearson@s comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a user action for stop in Android. This also splits out reload tracking to be from menu (phones) vs from toolbar (tablets), which is consistent with the other actions such as download. BUG=645294 ========== to ========== Add a user action for stop in Android. This also splits out reload tracking to be from menu (phones) vs from toolbar (tablets), which is consistent with the other actions such as download. BUG=645294 Committed: https://crrev.com/2f58462eb176193fd017c8a5e724436cb43d37b5 Cr-Commit-Position: refs/heads/master@{#419577} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2f58462eb176193fd017c8a5e724436cb43d37b5 Cr-Commit-Position: refs/heads/master@{#419577} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
