Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1895983002:
Adding UMA metrics to track usage of pointerevents. (Closed)
DescriptionAdding UMA metrics to track usage of pointerevents.
BUG=574898
TEST=virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html
Committed: https://crrev.com/763b6ccb8a2c42b95d8ca42b65d3e68ce522c19e
Cr-Commit-Position: refs/heads/master@{#389325}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 41 (15 generated)
Description was changed from ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 ========== to ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 ==========
sahel@chromium.org changed reviewers: + tim@chromium.org
tdresser@chromium.org changed reviewers: + tdresser@chromium.org - tim@chromium.org
Awesome, it looks great! I've updated the reviewer (sadly I'm not tim@chromium.org). To the CL description, add TEST=virtual/pointerevent/fast/events/pointerevents/pointer-use-count-expected.txt https://codereview.chromium.org/1895983002/diff/1/GOATS File GOATS (right): https://codereview.chromium.org/1895983002/diff/1/GOATS#newcode1 GOATS:1: Adding user Counters to count usage of PointerEvents. Why are there goats in this patch? https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:32: "gotpointercapture", "lostpointercapture"]; I don't think we need to test every event type for this test, as all event types are treated the same. Let's just send a single pointerenter. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:38: }); We don't need to explicitly test behavior of "lostpointercapture" in this test, so we can remove this. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:39: ["grey"].forEach(function(id) { This forEach looks unnecessary. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:47: targetDiv.setPointerCapture(event.pointerId); We don't need to test pointer capture logic in this test. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEvent.idl (right): https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEvent.idl:11: [MeasureAs=PointerEventCount] readonly attribute long pointerId; To measure any usage of an interface, it looks like the most common approach is to measure every attribute on the interface with the same use counter, e.g., https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 ========== to ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 TEST=virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html ==========
tdresser@chromium.org changed reviewers: + mustaq@chromium.org
+mustaq@ to make sure the pointer event specific logic makes sense. LGTM. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:35: if (event.eventPhase == Event.AT_TARGET) { We don't need to check the event phase here. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:37: if (event.type == "pointerdown") { We don't need to check the event type here.
I addressed the comments. https://codereview.chromium.org/1895983002/diff/1/GOATS File GOATS (right): https://codereview.chromium.org/1895983002/diff/1/GOATS#newcode1 GOATS:1: Adding user Counters to count usage of PointerEvents. On 2016/04/19 12:43:20, tdresser wrote: > Why are there goats in this patch? It's not in the new patch set. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:32: "gotpointercapture", "lostpointercapture"]; On 2016/04/19 12:43:20, tdresser wrote: > I don't think we need to test every event type for this test, as all event types > are treated the same. Let's just send a single pointerenter. I omitted some of the events from the list. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:38: }); On 2016/04/19 12:43:20, tdresser wrote: > We don't need to explicitly test behavior of "lostpointercapture" in this test, > so we can remove this. > Done. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:39: ["grey"].forEach(function(id) { On 2016/04/19 12:43:20, tdresser wrote: > This forEach looks unnecessary. Done. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:47: targetDiv.setPointerCapture(event.pointerId); On 2016/04/19 12:43:20, tdresser wrote: > We don't need to test pointer capture logic in this test. No setPointerCapture anymore, just accessing pointerID of the event to trigger the MeasureAs. https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEvent.idl (right): https://codereview.chromium.org/1895983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEvent.idl:11: [MeasureAs=PointerEventCount] readonly attribute long pointerId; On 2016/04/19 12:43:20, tdresser wrote: > To measure any usage of an interface, it looks like the most common approach is > to measure every attribute on the interface with the same use counter, e.g., > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... Done.
Thanks Sahel. I have one little request re the name of the counter: I think we could be more specific about the counter name. Who knows what other counter we would add for PointerEvents in future? The other comments are nits. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:9: #grey { Nit: could join the two style entries since there's a single div matching both. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:54: var PointerEventCount = 1306; Please add a comment to explain the number, e.g.: // From UseCounter.h https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEvent.idl (right): https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEvent.idl:11: [MeasureAs=PointerEventCount] readonly attribute long pointerId; Since this is counting attribute accesses, I would make the name more focused. What about "PointerEventAttributeCount" instead?
https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:9: #grey { On 2016/04/19 19:11:43, mustaq wrote: > Nit: could join the two style entries since there's a single div matching both. Done. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:35: if (event.eventPhase == Event.AT_TARGET) { On 2016/04/19 17:52:00, tdresser wrote: > We don't need to check the event phase here. Done. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:37: if (event.type == "pointerdown") { On 2016/04/19 17:52:00, tdresser wrote: > We don't need to check the event type here. I didn't want to print it for every event type. I can skip the debug (line 36) and get rid of the if clause in case it's better. https://codereview.chromium.org/1895983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:54: var PointerEventCount = 1306; On 2016/04/19 19:11:43, mustaq wrote: > Please add a comment to explain the number, e.g.: > // From UseCounter.h Done.
https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", "pointerup"]; On second thought, let's just add a single pointerdown listener. That lets us get rid of the loop over eventList, and the check if the type pointerdown.
https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", "pointerup"]; On 2016/04/20 12:04:54, tdresser wrote: > On second thought, let's just add a single pointerdown listener. > That lets us get rid of the loop over eventList, and the check if the type > pointerdown. Yes. And since the JS can check only the boolean result of counting, you can even nuke the last two eventSender.mouse*() calls. Tim, is there any way to useCount the PointerEvent interface itself? https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:51: debug(" **** Print PointerEventCount usage ***** "); s/PointerEventCount/PointerEventAttributeCount/
https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", "pointerup"]; On 2016/04/20 14:40:09, mustaq wrote: > On 2016/04/20 12:04:54, tdresser wrote: > > On second thought, let's just add a single pointerdown listener. > > That lets us get rid of the loop over eventList, and the check if the type > > pointerdown. > > Yes. And since the JS can check only the boolean result of counting, you can > even nuke the last two eventSender.mouse*() calls. > > Tim, is there any way to useCount the PointerEvent interface itself? I don't think there's a way to useCount the interface itself. It isn't entirely clear to me what that would even mean. Maybe we could usecount when people add pointer event event listeners?
On 2016/04/20 14:54:40, tdresser wrote: > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html > (right): > > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: > var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", > "pointerup"]; > On 2016/04/20 14:40:09, mustaq wrote: > > On 2016/04/20 12:04:54, tdresser wrote: > > > On second thought, let's just add a single pointerdown listener. > > > That lets us get rid of the loop over eventList, and the check if the type > > > pointerdown. > > > > Yes. And since the JS can check only the boolean result of counting, you can > > even nuke the last two eventSender.mouse*() calls. > > > > Tim, is there any way to useCount the PointerEvent interface itself? > > I don't think there's a way to useCount the interface itself. > > It isn't entirely clear to me what that would even mean. Neither to me, I was hoping to count "var pe = new PointerEvent(...)" in some way. > Maybe we could usecount when people add pointer event event listeners? This is definitely more useful. Let's discuss it separately---shouldn't block Sahel's very first CL. LGTM modulo a simpler test html without loops & with minimal eventSender calls.
On 2016/04/20 15:47:37, mustaq wrote: > On 2016/04/20 14:54:40, tdresser wrote: > > > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html > > (right): > > > > > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: > > var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", > > "pointerup"]; > > On 2016/04/20 14:40:09, mustaq wrote: > > > On 2016/04/20 12:04:54, tdresser wrote: > > > > On second thought, let's just add a single pointerdown listener. > > > > That lets us get rid of the loop over eventList, and the check if the type > > > > pointerdown. > > > > > > Yes. And since the JS can check only the boolean result of counting, you can > > > even nuke the last two eventSender.mouse*() calls. > > > > > > Tim, is there any way to useCount the PointerEvent interface itself? > > > > I don't think there's a way to useCount the interface itself. > > > > It isn't entirely clear to me what that would even mean. > > Neither to me, I was hoping to count "var pe = new PointerEvent(...)" in some > way. > > > Maybe we could usecount when people add pointer event event listeners? > > This is definitely more useful. Let's discuss it separately---shouldn't block > Sahel's very first CL. > > LGTM modulo a simpler test html without loops & with minimal eventSender calls. I don't think it's "definitely more useful". It may be more useful, but I'm not 100% sure. How often do you think people add a pointer event listener but never actually pay any attention to the events produced? I suppose it'll happen if you're just trying to measure user interaction. What are we actually trying to measure here? If it's adoption of pointer events, I think attribute access is probably a reasonable metric. We can measure the constructor too, which I didn't realize. There's an example here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
On 2016/04/20 16:03:04, tdresser wrote: > On 2016/04/20 15:47:37, mustaq wrote: > > On 2016/04/20 14:54:40, tdresser wrote: > > > > > > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > > > File > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: > > > var eventList = ["pointerover", "pointerenter", "pointermove", > "pointerdown", > > > "pointerup"]; > > > On 2016/04/20 14:40:09, mustaq wrote: > > > > On 2016/04/20 12:04:54, tdresser wrote: > > > > > On second thought, let's just add a single pointerdown listener. > > > > > That lets us get rid of the loop over eventList, and the check if the > type > > > > > pointerdown. > > > > > > > > Yes. And since the JS can check only the boolean result of counting, you > can > > > > even nuke the last two eventSender.mouse*() calls. > > > > > > > > Tim, is there any way to useCount the PointerEvent interface itself? > > > > > > I don't think there's a way to useCount the interface itself. > > > > > > It isn't entirely clear to me what that would even mean. > > > > Neither to me, I was hoping to count "var pe = new PointerEvent(...)" in some > > way. > > > > > Maybe we could usecount when people add pointer event event listeners? > > > > This is definitely more useful. Let's discuss it separately---shouldn't block > > Sahel's very first CL. > > > > LGTM modulo a simpler test html without loops & with minimal eventSender > calls. > > I don't think it's "definitely more useful". It may be more useful, but I'm not > 100% sure. > > How often do you think people add a pointer event listener but never actually > pay any attention to the events produced? > I suppose it'll happen if you're just trying to measure user interaction. > > What are we actually trying to measure here? If it's adoption of pointer events, > I think attribute access is probably a reasonable metric. > > We can measure the constructor too, which I didn't realize. There's an example > here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... Let's discuss it separately because I seem to have a different opinion.
I narrowed down the events to "pointerdown". deleted the last to eventSender calls. (I needed the first one to move to the grey div). I also had to change the enum number because meanwhile 3-4 other counters were added to the code. https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", "pointerup"]; On 2016/04/20 12:04:54, tdresser wrote: > On second thought, let's just add a single pointerdown listener. > That lets us get rid of the loop over eventList, and the check if the type > pointerdown. Done. https://codereview.chromium.org/1895983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:28: var eventList = ["pointerover", "pointerenter", "pointermove", "pointerdown", "pointerup"]; On 2016/04/20 14:40:09, mustaq wrote: > On 2016/04/20 12:04:54, tdresser wrote: > > On second thought, let's just add a single pointerdown listener. > > That lets us get rid of the loop over eventList, and the check if the type > > pointerdown. > > Yes. And since the JS can check only the boolean result of counting, you can > even nuke the last two eventSender.mouse*() calls. > > Tim, is there any way to useCount the PointerEvent interface itself? Done.
tdresser@chromium.org changed reviewers: + bokan@chromium.org, isherman@chromium.org
+bokan@ for OWNERS for webkit. +isherman@ for histograms.xml. https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:42: var PointerEventAttributeCount = 1306; //Comes from enum Feature in UseCounter.h This didn't get updated.
https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:44: debug(internals.isUseCounted(document, PointerEventAttributeCount)); It's more idiomatic to check your assertions using: shouldBe('true', 'internals.isUseCounted(document, PointerEventAttributeCount)'); // Note: arguments are strings, the get evaluated to JS. You'll need to move PointerEventAttributeCount to be global though. https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1125: MediaStreamTrackRemote = 1306, Presumably these aren't part of your patch? Bad rebase? https://codereview.chromium.org/1895983002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1895983002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:381: +<histogram name="Android.MultiWindowMode.Active" enum="BooleanEnabled"> Ditto on this file, it looks like a bunch of other changes are included here as well.
https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1125: MediaStreamTrackRemote = 1306, On 2016/04/21 13:12:11, bokan wrote: > Presumably these aren't part of your patch? Bad rebase? Oops, good catch. I thought that was just histograms.xml being crazy.
https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:42: var PointerEventAttributeCount = 1306; //Comes from enum Feature in UseCounter.h On 2016/04/21 12:15:59, tdresser wrote: > This didn't get updated. Done. https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:44: debug(internals.isUseCounted(document, PointerEventAttributeCount)); On 2016/04/21 13:12:11, bokan wrote: > It's more idiomatic to check your assertions using: > > shouldBe('true', 'internals.isUseCounted(document, > PointerEventAttributeCount)'); // Note: arguments are strings, the get evaluated > to JS. > > You'll need to move PointerEventAttributeCount to be global though. Done. https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1125: MediaStreamTrackRemote = 1306, On 2016/04/21 13:23:08, tdresser wrote: > On 2016/04/21 13:12:11, bokan wrote: > > Presumably these aren't part of your patch? Bad rebase? > > Oops, good catch. I thought that was just histograms.xml being crazy. fixed now. https://codereview.chromium.org/1895983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1125: MediaStreamTrackRemote = 1306, On 2016/04/21 13:12:11, bokan wrote: > Presumably these aren't part of your patch? Bad rebase? Done. https://codereview.chromium.org/1895983002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1895983002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:381: +<histogram name="Android.MultiWindowMode.Active" enum="BooleanEnabled"> On 2016/04/21 13:12:11, bokan wrote: > Ditto on this file, it looks like a bunch of other changes are included here as > well. I had a problem with master branch, and patch branch. It's fixed now.
Thanks, webkit lgtm % minor nit https://codereview.chromium.org/1895983002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:42: shouldBe('true', 'internals.isUseCounted(document, PointerEventAttributeCount)'); nit: sorry, I misled you, the value being tested should be first, the expectation second so please flip the order of arguments here.
https://codereview.chromium.org/1895983002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html (right): https://codereview.chromium.org/1895983002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html:42: shouldBe('true', 'internals.isUseCounted(document, PointerEventAttributeCount)'); On 2016/04/21 20:18:16, bokan wrote: > nit: sorry, I misled you, the value being tested should be first, the > expectation second so please flip the order of arguments here. No worries, fixed it.
histograms.xml lgtm
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, mustaq@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1895983002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895983002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895983002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, isherman@chromium.org, tdresser@chromium.org, mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/1895983002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895983002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895983002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sahel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895983002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895983002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 TEST=virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html ========== to ========== Adding UMA metrics to track usage of pointerevents. BUG=574898 TEST=virtual/pointerevent/fast/events/pointerevents/pointer-use-count.html Committed: https://crrev.com/763b6ccb8a2c42b95d8ca42b65d3e68ce522c19e Cr-Commit-Position: refs/heads/master@{#389325} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/763b6ccb8a2c42b95d8ca42b65d3e68ce522c19e Cr-Commit-Position: refs/heads/master@{#389325} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
