Closed
Bug 1340904
Opened 7 years ago
Closed 7 years ago
Rewrite the telemetry scroll probes in C++
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: rhunt, Assigned: rhunt)
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 1 obsolete file)
13.05 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
After bug 1338891, bug 1297867 and bug 1312881 were backed out of beta, aurora, and central. This bug is for rewriting the scroll probes in c++ in a way that does no significantly affect performance.
Assignee | ||
Comment 1•7 years ago
|
||
An evolution of the previous patch. The max scroll and amount scroll metrics are updated by FireScrollEvent in nsGfxScrollFrame and stored in the PresContext of any root scroll frame. The metrics are submitted by TelemetryScrollProbe which listens for 'pagehide' events coming from the frame script global. I've verified that this patch accurately records the same metric that the original JS probe did. This approach isn't the cleanest, but it seems like it is sensible. Please let me know if there is a better/cleaner way. Thanks for all your help!
Attachment #8842749 -
Flags: review?(bugs)
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3a8e36c281ecb524a8ef7b4163a2399473bb360 It looks like this patch is leaking the world, and potentially has a crash in it.
Comment 3•7 years ago
|
||
Comment on attachment 8842749 [details] [diff] [review] scroll-probe.patch >+/* static */ void >+TelemetryScrollProbe::Create(TabChildGlobal* aWebFrame) { Nit, { goes to its own line after method definition. Same thing also elsewhere. But I'll re-read once the leak is fixed.
Attachment #8842749 -
Flags: review?(bugs)
Comment 4•7 years ago
|
||
Could you use nsIDocument::GetOriginalURI and not Location(). And it isn't clear to me why about:blank is filtered out. And using nsAutoString on stack is probably faster than nsString.
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f59f36b971c5e12a335d38891ec440e5d6594c It looks like the leak has been cleaned up.
Attachment #8842749 -
Attachment is obsolete: true
Attachment #8843072 -
Flags: review?(bugs)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #4) > And it isn't clear to me why about:blank is filtered out. about:blank, about:newtab, etc. are excluded from this probe so that we focus on web page interaction that isn't 'chrome' or controlled by firefox. We're not sure if that's important or not, but it's not hard to test for.
Comment 7•7 years ago
|
||
about:blank in general isn't content or chrome. Web pages do use about:blank all the time. Maybe you want principal check and store data only about non-system-principal documents? !nsContentUtils::IsSystemPrincipal(nsIDocument_object->NodePrincipal()) (and about:newtab seems to run in the parent process. )
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #7) > about:blank in general isn't content or chrome. Web pages do use about:blank > all the time. > Maybe you want principal check and store data only about > non-system-principal documents? > !nsContentUtils::IsSystemPrincipal(nsIDocument_object->NodePrincipal()) > > > (and about:newtab seems to run in the parent process. ) Ah yeah, you're right I didn't mean chrome in the strict sense. But I didn't know web pages actually use about:blank, interesting. A principal check could be good. That could work better than looking at the scheme.
Comment 9•7 years ago
|
||
Comment on attachment 8843072 [details] [diff] [review] scroll-probe.patch >+bool >+TelemetryScrollProbe::ShouldIgnore(nsIDOMEvent* aEvent) const >+{ >+ nsCOMPtr<nsIDOMEventTarget> target; >+ aEvent->GetTarget(getter_AddRefs(target)); >+ nsCOMPtr<nsIDocument> targetDocument = do_QueryInterface(target); >+ RefPtr<nsIDocument> document = GetDocument(); >+ >+ // only respond to the root, ignore 'pagehide' from iframes >+ if (!document || targetDocument != document) { >+ return false; >+ } >+ >+ // skip about:// pages for this probe >+ nsCOMPtr<nsIURI> location = document->GetOriginalURI(); >+ nsAutoCString scheme; >+ return !location || NS_FAILED(location->GetScheme(scheme)) || scheme.EqualsLiteral("about"); >+} So I think we can simplify this a bit and just have (I think this captures the meaning well enough) if (!document || targetDocument != document || nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) { return false; } return true; >+ void SetTelemetryScrollY(nscoord aScrollY) { >+ nscoord delta = abs(aScrollY - mTelemetryScrollLastY); >+ mTelemetryScrollLastY = aScrollY; >+ >+ mTelemetryScrollTotalY += delta; >+ if (aScrollY > mTelemetryScrollMaxY) { >+ mTelemetryScrollMaxY = aScrollY; >+ } >+ } >+ nscoord TelemetryScrollMaxY() const { >+ return mTelemetryScrollMaxY; >+ } >+ nscoord TelemetryScrollTotalY() const { >+ return mTelemetryScrollTotalY; >+ } I know some of nsPresContext uses wrong coding style, but { after method definition should go to its own line
Attachment #8843072 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•7 years ago
|
||
Testing with (In reply to Olli Pettay [:smaug] from comment #9) > Comment on attachment 8843072 [details] [diff] [review] > scroll-probe.patch > > So I think we can simplify this a bit and just have (I think this captures > the meaning well enough) > > if (!document || targetDocument != document || > nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) { > return false; > } > > return true; Hm. So running this code (inverted the condition because it's ShouldIgnore()) I'm getting about: pages recorded. So that's making me guess that about: pages are not running under the system principal? Is there any other way to detect pages in about:, or is looking at the URI the best shot?
Flags: needinfo?(bugs)
Comment 11•7 years ago
|
||
some about: pages use system principal (at least in parent process, but that isn't relevant here), and some don't. about:blank inherits principal from load triggerer, and in content-docshell case it is by default non-system principal. which about:* which doesn't have system principal are you worried about? We can leave the explicit about: check there too, but it feels a bit wrong. And oops, it was ShouldIgnore, so !nsContentUtils::IsSystemPrincipal(document->NodePrincipal()) Though, I think !document should be ignored
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11) > which about:* which doesn't have system principal are you worried about? I think the concern is only about biasing towards 0 with this metric because every user has to go through about:home, which generally doesn't scroll. But, this whole metric is hypothetical if it will be even useful, and any noise from about:home will be before Quantum changes and after. So I'd rather not worry much about it, and just stick with checking for the system principal. I'm going to do some more testing on this to make sure it's okay before landing, I'd rather not cause a third problem with this probe.
Comment 13•7 years ago
|
||
> So I'd rather not worry much about it, and just stick with checking for the system principal.
+1, bias from about: is not expected to cause noise and if people engage on them more it might even be interesting to know.
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ba15fb30b0456a00a6fbad8d52e77db39abbf64&selectedJob=81517700 It looks green to me.
Assignee | ||
Comment 15•7 years ago
|
||
Harald, should we get this landed or is there anything you want to wait for or discuss?
Flags: needinfo?(hkirschner)
Comment 16•7 years ago
|
||
Looks good to me. Need to have this landed to see how it looks in the wild.
Flags: needinfo?(hkirschner)
Comment 17•7 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/11b27d2c87e7 Implement telemetry scroll tracking in C++ r=smaug
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11b27d2c87e7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 19•7 years ago
|
||
Did these new probes receive data-review from a Data Peer per https://wiki.mozilla.org/Firefox/Data_Collection ?
Comment 20•7 years ago
|
||
(For the record I see that the backed-out bugs _did_ receive data-review, but these new ones appear unreviewed)
Assignee | ||
Comment 21•7 years ago
|
||
No the new patches did not receive data-review. The new patches have a different implementation, but measure the same metric as before. The histograms are the exact same as the ones backed out, so no descriptions or expire date change. I didn't think about data-review, do we need it in this case?
Flags: needinfo?(benjamin)
Comment 22•7 years ago
|
||
If the histogram is the same and has the same meaning then you don't need a data review.
Flags: needinfo?(benjamin)
You need to log in
before you can comment on or make changes to this bug.
Description
•