Closed Bug 1388738 Opened 7 years ago Closed 7 years ago

Call devtools-startup initDevTools from DevToolsShim

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox56 verified, firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(2 files)

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 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 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 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
We'll need to uplift this so it can catch up to bug 1378863
https://hg.mozilla.org/mozilla-central/rev/b6f95025c1ee
https://hg.mozilla.org/mozilla-central/rev/387b09897512
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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?
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.
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)
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
(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.
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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+
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.