Closed
Bug 1388738
Opened 7 years ago
Closed 7 years ago
Call devtools-startup initDevTools from DevToolsShim
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(firefox56 verified, firefox57 verified)
VERIFIED
FIXED
Firefox 57
People
(Reporter: jdescottes, Assigned: jdescottes)
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
devtools-startup::initDevTools takes care of interfacing with telemetry to register how the user first interacted with DevTools. The context menu item "Inspect Element" call the DevToolsShim. At the moment the shim duplicated the initDevTools logic instead of calling devtools-startup. We should fix that so that the context-menu entry point get registered in telemetry. The "ContextMenu" category is already created for the Telemetry probe we are using here (http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/toolkit/components/telemetry/Histograms.json#9337-9346). The goal of this bug is simply to load devtools-startup from the DevToolsShim and call initDevTools("ContextMenu") when needed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8895475 [details] Bug 1388738 - reuse devtools-startup initDevTools in DevToolsShim; https://reviewboard.mozilla.org/r/166596/#review172238 ::: devtools/shim/DevToolsShim.jsm:249 (Diff revision 2) > + * optional, if provided should be a valid entry point for DEVTOOLS_ENTRY_POINT > + * in toolkit/components/telemetry/Histograms.json > + */ > + _initDevTools: function (reason) { > + const devtoolsStartup = Cc["@mozilla.org/devtools/startup-clh;1"] > + .getService(Ci.nsICommandLineHandler) nit: would be great to use XPCOMUtils to get the service only once. you can use defineLazyGetter to unwrap it.
Attachment #8895475 -
Flags: review?(poirot.alex) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8895758 [details] Bug 1388738 - add comment for devtools-startup developerToggleCreated flag; https://reviewboard.mozilla.org/r/167090/#review172242
Attachment #8895758 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895475 [details] Bug 1388738 - reuse devtools-startup initDevTools in DevToolsShim; https://reviewboard.mozilla.org/r/166596/#review172238 > nit: would be great to use XPCOMUtils to get the service only once. you can use defineLazyGetter to unwrap it. Thanks for the review! Done.
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6f95025c1ee reuse devtools-startup initDevTools in DevToolsShim;r=ochameau https://hg.mozilla.org/integration/autoland/rev/387b09897512 add comment for devtools-startup developerToggleCreated flag;r=ochameau
Comment 10•7 years ago
|
||
We'll need to uplift this so it can catch up to bug 1378863
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6f95025c1ee https://hg.mozilla.org/mozilla-central/rev/387b09897512
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8895475 [details] Bug 1388738 - reuse devtools-startup initDevTools in DevToolsShim; Approval Request Comment [Feature/Bug causing the regression]: Bug 1378863 (not really a regression, but this is a follow-up) [User impact if declined]: missing telemetry data for a new probe [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: Not yet [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: Minor javascript change, devtools only. We mutualized duplicated code, but didn't change the logic. [String changes made/needed]: none
Attachment #8895475 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 13•7 years ago
|
||
To verify the bug (not on Nightly yet!): - open FF with a clean profile - open about:telemetry - select "Histograms" in the side bar - type "DEVTOOLS_ENTRY_POINT" in the filter => no histogram should be displayed - open a new tab - right click anywhere on the page, select "Inspect Element" to open DevTools - go back to about:telemetry - refresh the about:telemetry page - the histogram for "DEVTOOLS_ENTRY_POINT" should be displayed, with 1 hit for category 3.
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 14•7 years ago
|
||
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Comment 15•7 years ago
|
||
Not sure if this is enough to verify, but this caused a big enough difference in the telemetry probe DEVTOOLS_ENTRY_POINT that the automated alerts caught it: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/2463/alerts/?from=2017-08-11&to=2017-08-11
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #15) > Not sure if this is enough to verify, but this caused a big enough > difference in the telemetry probe DEVTOOLS_ENTRY_POINT that the automated > alerts caught it: > http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/2463/ > alerts/?from=2017-08-11&to=2017-08-11 Yes, and related to that I confirmed that this was an expected outcome of the change on the devtools mailing list.
Comment 17•7 years ago
|
||
I managed to reproduce the issue on an older Nightly 57.0a1 (2017-08-09) build under Windows 10 x64. Verified as fixed using latest Nightly 57.0a1 (2017-08-18) on Ubuntu 14.04 x64, Windows 10 x64 and Mac OS X 10.12.6
Comment 18•7 years ago
|
||
Comment on attachment 8895475 [details] Bug 1388738 - reuse devtools-startup initDevTools in DevToolsShim; Fix a telemetry issue related to devtools and was verified. Beta56+.
Attachment #8895475 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ba9cc5295263 https://hg.mozilla.org/releases/mozilla-beta/rev/e9b8e3fb359c
Comment 20•7 years ago
|
||
I was able to reproduce the initial issue on 56.0b4 build5 (20170819205558). I can confirm the bug is verified fixed on 56.0b5 build1 (20170821193225), using Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.12.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•