Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628943003:
Add metrics for triggering of JS dialogs on Android. (Closed)
DescriptionAdd metrics for triggering of JS dialogs on Android.
These match the existing metrics logged for desktop
added in:
https://codereview.chromium.org/2502493002
BUG=629964
Review-Url: https://codereview.chromium.org/2628943003
Cr-Commit-Position: refs/heads/master@{#445106}
Committed: https://chromium.googlesource.com/chromium/src/+/ee107748dd959954722d9c4cf56c3ce1988d2c7e
Patch Set 1 #
Total comments: 3
Patch Set 2 : Log only for confirm dialog #
Total comments: 2
Patch Set 3 : Rebase and updated comment #
Messages
Total messages: 19 (4 generated)
tedchoc@chromium.org changed reviewers: + avi@chromium.org, dfalcantara@chromium.org
dfalcantara - ptal avi - for metrics question in javascript_app_modal_dialog_android https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( @avi: are beforeunload handlers triggered differently on desktop? should we only log the above for the else case?
This makes me wonder. Is your "foremost" equivalent == "the tab that is on top of everything else"? Desktop's "foremost" is "the foreground tab of the frontmost window". If that's the same, cool, but if it's not then maybe we shouldn't reuse this. https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( On 2017/01/12 01:11:39, Ted C wrote: > @avi: are beforeunload handlers triggered differently on desktop? should we > only log the above for the else case? beforeunload is a different code path on desktop; this is used only for window.confirm. Definitely the else path here.
On 2017/01/12 01:25:39, Avi (offsites OOO 17Jan-2Feb) wrote: > This makes me wonder. > > Is your "foremost" equivalent == "the tab that is on top of everything else"? > Desktop's "foremost" is "the foreground tab of the frontmost window". If that's > the same, cool, but if it's not then maybe we shouldn't reuse this. Android is primarily a single window system. Even in multi-window, they are still split screen only. So if you have two Chrome windows in multi-window, they would both be considered foremost using the logic I added. We could also consider foremost to be the last focused activity in this situation, but the difference would be negligible for sure. If you think we should add our own metric, that is also fine. > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > On 2017/01/12 01:11:39, Ted C wrote: > > @avi: are beforeunload handlers triggered differently on desktop? should we > > only log the above for the else case? > > beforeunload is a different code path on desktop; this is used only for > window.confirm. Definitely the else path here.
https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > On 2017/01/12 01:11:39, Ted C wrote: > > @avi: are beforeunload handlers triggered differently on desktop? should we > > only log the above for the else case? > > beforeunload is a different code path on desktop; this is used only for > window.confirm. Definitely the else path here. Moved to the else
On 2017/01/13 17:32:05, Ted C wrote: > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > > On 2017/01/12 01:11:39, Ted C wrote: > > > @avi: are beforeunload handlers triggered differently on desktop? should we > > > only log the above for the else case? > > > > beforeunload is a different code path on desktop; this is used only for > > window.confirm. Definitely the else path here. > > Moved to the else For Android, "foremost" would mean for me the page the user is actively looking at, so whatever achieves that feels right.
On 2017/01/13 17:49:12, Avi (offsites OOO 17Jan-2Feb) wrote: > On 2017/01/13 17:32:05, Ted C wrote: > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right): > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > > On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > > > On 2017/01/12 01:11:39, Ted C wrote: > > > > @avi: are beforeunload handlers triggered differently on desktop? should > we > > > > only log the above for the else case? > > > > > > beforeunload is a different code path on desktop; this is used only for > > > window.confirm. Definitely the else path here. > > > > Moved to the else > > For Android, "foremost" would mean for me the page the user is actively looking > at, so whatever achieves that feels right. I'd argue that in non-freeform multi-window both are actively being looked at in this scenario. Here they both are fully active, you can interact with both, so this seems right to me.
On 2017/01/13 17:54:34, Ted C wrote: > On 2017/01/13 17:49:12, Avi (offsites OOO 17Jan-2Feb) wrote: > > On 2017/01/13 17:32:05, Ted C wrote: > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc > (right): > > > > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > > > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > > > On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > > > > On 2017/01/12 01:11:39, Ted C wrote: > > > > > @avi: are beforeunload handlers triggered differently on desktop? > should > > we > > > > > only log the above for the else case? > > > > > > > > beforeunload is a different code path on desktop; this is used only for > > > > window.confirm. Definitely the else path here. > > > > > > Moved to the else > > > > For Android, "foremost" would mean for me the page the user is actively > looking > > at, so whatever achieves that feels right. > > I'd argue that in non-freeform multi-window both are actively being looked at in > this > scenario. Here they both are fully active, you can interact with both, so this > seems > right to me. Ugh. You mean side-by-side? In the desktop world, there can only be one front window, so we're calling the tab in that foremost. Even if windows are side-by-side, we have the issue of locking up a renderer if they share it, so I don't know what the approach is here.
On 2017/01/13 17:58:45, Avi (offsites OOO 17Jan-2Feb) wrote: > On 2017/01/13 17:54:34, Ted C wrote: > > On 2017/01/13 17:49:12, Avi (offsites OOO 17Jan-2Feb) wrote: > > > On 2017/01/13 17:32:05, Ted C wrote: > > > > > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > > > > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > > > > On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > > > > > On 2017/01/12 01:11:39, Ted C wrote: > > > > > > @avi: are beforeunload handlers triggered differently on desktop? > > should > > > we > > > > > > only log the above for the else case? > > > > > > > > > > beforeunload is a different code path on desktop; this is used only for > > > > > window.confirm. Definitely the else path here. > > > > > > > > Moved to the else > > > > > > For Android, "foremost" would mean for me the page the user is actively > > looking > > > at, so whatever achieves that feels right. > > > > I'd argue that in non-freeform multi-window both are actively being looked at > in > > this > > scenario. Here they both are fully active, you can interact with both, so > this > > seems > > right to me. > > Ugh. You mean side-by-side? Yeah https://developer.android.com/guide/topics/ui/multi-window.html > > In the desktop world, there can only be one front window, so we're calling the > tab in that foremost. Even if windows are side-by-side, we have the issue of > locking up a renderer if they share it, so I don't know what the approach is > here. Desktop has a much bolder "focused" window UX. Even in side by side, the other window will be dimmed. That isn't the case on Android and they both seem "foremost", but multiwindow is only in the newest android so it doesn't have super high penetration yet. I think if we start talking about behavior we might need to do something different, but for metrics it is a bit murky IMO. But if you feel strongly, I can make it exactly the same on Android (it just seems slightly weird but not overly so)
On 2017/01/13 18:04:16, Ted C wrote: > On 2017/01/13 17:58:45, Avi (offsites OOO 17Jan-2Feb) wrote: > > On 2017/01/13 17:54:34, Ted C wrote: > > > On 2017/01/13 17:49:12, Avi (offsites OOO 17Jan-2Feb) wrote: > > > > On 2017/01/13 17:32:05, Ted C wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > > > File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2628943003/diff/1/chrome/browser/ui/android/j... > > > > > chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:75: > > > > > dialog_object = Java_JavascriptAppModalDialog_createBeforeUnloadDialog( > > > > > On 2017/01/12 01:25:38, Avi (offsites OOO 17Jan-2Feb) wrote: > > > > > > On 2017/01/12 01:11:39, Ted C wrote: > > > > > > > @avi: are beforeunload handlers triggered differently on desktop? > > > should > > > > we > > > > > > > only log the above for the else case? > > > > > > > > > > > > beforeunload is a different code path on desktop; this is used only > for > > > > > > window.confirm. Definitely the else path here. > > > > > > > > > > Moved to the else > > > > > > > > For Android, "foremost" would mean for me the page the user is actively > > > looking > > > > at, so whatever achieves that feels right. > > > > > > I'd argue that in non-freeform multi-window both are actively being looked > at > > in > > > this > > > scenario. Here they both are fully active, you can interact with both, so > > this > > > seems > > > right to me. > > > > Ugh. You mean side-by-side? > > Yeah > > https://developer.android.com/guide/topics/ui/multi-window.html > > > > > In the desktop world, there can only be one front window, so we're calling the > > tab in that foremost. Even if windows are side-by-side, we have the issue of > > locking up a renderer if they share it, so I don't know what the approach is > > here. > > Desktop has a much bolder "focused" window UX. Even in side by side, the > other window will be dimmed. That isn't the case on Android and they both > seem "foremost", but multiwindow is only in the newest android so it doesn't > have super high penetration yet. > > I think if we start talking about behavior we might need to do something > different, but for metrics it is a bit murky IMO. But if you feel strongly, > I can make it exactly the same on Android (it just seems slightly weird > but not overly so) I'm not asking for a change in behavior right now, I'm just worried that if the userfeel is different, it might not be appropriate to reuse the UMA name because it might not be measuring quite the same thing.
https://codereview.chromium.org/2628943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2628943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2244: * something like the phone tab switcher). How does this work for a tablet, where the tab might be dead but is still visible on the strip? Maybe the comment can be clarified so that it implies that the user is interacting with the web page displayed in the tab?
https://codereview.chromium.org/2628943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2628943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2244: * something like the phone tab switcher). On 2017/01/13 18:54:37, dfalcantara (load balance plz) wrote: > How does this work for a tablet, where the tab might be dead but is still > visible on the strip? Maybe the comment can be clarified so that it implies > that the user is interacting with the web page displayed in the tab? Updated the comment to clarify the tab's view needs to be visible (since it doesn't necessarily need to be web contents).
lgtm
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484935850230940,
"parent_rev": "5c363ea4a04878fd491b6e674f802deff42245f6", "commit_rev":
"ee107748dd959954722d9c4cf56c3ce1988d2c7e"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics for triggering of JS dialogs on Android. These match the existing metrics logged for desktop added in: https://codereview.chromium.org/2502493002 BUG=629964 ========== to ========== Add metrics for triggering of JS dialogs on Android. These match the existing metrics logged for desktop added in: https://codereview.chromium.org/2502493002 BUG=629964 Review-Url: https://codereview.chromium.org/2628943003 Cr-Commit-Position: refs/heads/master@{#445106} Committed: https://chromium.googlesource.com/chromium/src/+/ee107748dd959954722d9c4cf56c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ee107748dd959954722d9c4cf56c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
