Closed
Bug 1386821
Opened 7 years ago
Closed 7 years ago
Developer toolbar not displayed when firefox first starts up
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
Tracking
(firefox-esr52 unaffected, firefox55 unaffected, firefox56 verified, firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | verified |
firefox57 | --- | verified |
People
(Reporter: wlach, Assigned: jdescottes)
References
Details
(Keywords: regression)
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
4.41 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
Reproduction steps: Start firefox with `devtools.toolbar.visible` set to TRUE. Expected: The "developer toolbar" (shift-f2) is present on completed startup. Actual: The "developer toolbar" (shift-f2) is not present. If I browse into tools->web developer, it suddenly appears. Doing a bisection with mozregression (mozregression --pref "devtools.toolbar.visible:true" -g 2017-07-01 -b 2017-08-01), I got this pushlog: 7:42.83 INFO: Last good revision: f91d86bb4b2adad09711ce55c82c8d695b8e6ed7 7:42.83 INFO: First bad revision: 1b4dea0fe0213d36833103764921046ec41bcca6 7:42.83 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f91d86bb4b2adad09711ce55c82c8d695b8e6ed7&tochange=1b4dea0fe0213d36833103764921046ec41bcca6 It appears as if bug 1359855 is the culprit. Alexandre, any ideas?
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
This pref is used for redisplaying the developer toolbar on every browser restart, so we might want to uplift this. Also we have been discussing about adding an entry point test for this kind of issues. Alex: I think we need to start Firefox from the test in order to assert things such as devtools.toolbar.visible. Do you know if this is possible?
Comment 3•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #2) > This pref is used for redisplaying the developer toolbar on every browser > restart, so we might want to uplift this. > > Also we have been discussing about adding an entry point test for this kind > of issues. > Alex: I think we need to start Firefox from the test in order to assert > things such as devtools.toolbar.visible. Do you know if this is possible? Not that I am aware of. mochitests always open a first browser window. And xpcshell doesn't involve devtools-startup, nor would it support opening a browser window. May be Talos addons can do that, but that's not really the right purpose. In most cases, we test this kind of behavior by opening a new browser window: http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js#50-51 let win = OpenBrowserWindow(); yield waitForDelayedStartupFinished(win); That wouldn't ensure that the behavior is correct at startup, but that should cover this code. You should be able to check somehow that initDevTools is called when opening a new window when the pref is on...
Flags: needinfo?(poirot.alex)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8893233 [details] Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; https://reviewboard.mozilla.org/r/164274/#review169658 ::: devtools/client/devtools-startup.js:196 (Diff revision 1) > > // Only top level Firefox Windows fire a browser-delayed-startup-finished event > let onWindowReady = window => { > this.hookWindow(window); > > + if (Services.prefs.getBoolPref(TOOLBAR_VISIBLE_PREF, false)) { This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref? Or do such safe check before calling getBoolPref: if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref) ::: devtools/client/devtools-startup.js:369 (Diff revision 1) > * By initialized, we mean that its main modules are loaded. > */ > initialized: false, > > initDevTools: function (reason) { > if (!this.initialized) { If you don't pass a reason from onWindowReady, you should change this check here to: if (!this.initalized && reason) {
Attachment #8893233 -
Flags: review?(poirot.alex) → review+
Updated•7 years ago
|
Assignee: nobody → jdescottes
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893233 [details] Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; https://reviewboard.mozilla.org/r/164274/#review169658 > This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref? > Or do such safe check before calling getBoolPref: > if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref) Since we pass a default value, this call should never throw, even if the pref is not defined. If devtools are not installed, then it's ok to get the "false" default value. The question is, if devtools are installed, do we process the devtools prefs in the addon before doing this check or not. I think you're right and we should simply be safe and move the pref to the startup preferences here. I'll block this on Bug 1386620 > If you don't pass a reason from onWindowReady, you should change this check here to: > if (!this.initalized && reason) { good catch!
Comment 6•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #5) > Comment on attachment 8893233 [details] > Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; > > https://reviewboard.mozilla.org/r/164274/#review169658 > > > This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref? > > Or do such safe check before calling getBoolPref: > > if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref) > > Since we pass a default value, this call should never throw, even if the > pref is not defined. > If devtools are not installed, then it's ok to get the "false" default > value. Oh. I didn't knew that getBoolPref now support a default value. That's handy!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893233 [details] Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; https://reviewboard.mozilla.org/r/164274/#review169658 > good catch! I decided to move this.initialized = true inside of the if (reason && !initialized) block. The reasoning is that for devtools.toolbar.visible = true, even though it requires devtools tobe initialized, it's not really an interesting entry point. Even if the user has the toolbar opening by default we still want to know how they actually open the toolbox. Let me know if you're ok with that.
Comment 9•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #8) > Comment on attachment 8893233 [details] > Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; > > https://reviewboard.mozilla.org/r/164274/#review169658 > > > good catch! > > I decided to move this.initialized = true inside of the if (reason && > !initialized) block. You goal makes sense, but you can't modify `initialized` behavior like this. First, the name of this property would be very misleading, but it also breaks its usage over here: http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#211-214 Please introduce yet another flag to do what you want regarding telemetry here.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #9) > (In reply to Julian Descottes [:jdescottes] from comment #8) > > Comment on attachment 8893233 [details] > > Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; > > > > https://reviewboard.mozilla.org/r/164274/#review169658 > > > > > good catch! > > > > I decided to move this.initialized = true inside of the if (reason && > > !initialized) block. > > You goal makes sense, but you can't modify `initialized` behavior like this. > First, the name of this property would be very misleading, but it also > breaks its usage over here: > http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup. > js#211-214 > Please introduce yet another flag to do what you want regarding telemetry > here. Ah missed this usage of this.initialized. Since it was defined in just above initDevTools() I assumed it was only used there. Moved this.initialized back out of the if, and adding a second flag in another changeset.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8893340 [details] Bug 1386821 - add a separate flag to check if devtools entry point was recorded; https://reviewboard.mozilla.org/r/164434/#review170806 ::: devtools/shim/devtools-startup.js:177 (Diff revision 2) > + initialized: false, > + > + /** > + * Boolean flag to check if the devtools initialization was already sent to telemetry. > + * We only want to record one devtools entry point per Firefox run, but we are not > + * interested in all the entry points (e.g. devtools.toolbar.visible. nit: missing `)` at the end.
Attachment #8893340 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da2bc55ad33a call initDevTools if devtools.toolbar.visible is true;r=ochameau https://hg.mozilla.org/integration/autoland/rev/e38283010bb3 add a separate flag to check if devtools entry point was recorded;r=ochameau
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/da2bc55ad33a https://hg.mozilla.org/mozilla-central/rev/e38283010bb3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 20•7 years ago
|
||
Please nominate this for Beta approval when you get a chance.
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(jdescottes)
Version: unspecified → 56 Branch
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8893233 [details] Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; Approval Request Comment [Feature/Bug causing the regression]: Bug 1359855 [User impact if declined]: The devtools command line is no longer re-opening on startup, forcing users to reenable it every time. [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]: yes. STRs: - open Firefox - open DevTools Command Line (SHIFT+F2) - quit/re-open Firefox [List of other uplifts needed for the feature/fix]: (we should uplift the patch from Bug 1387359 first to avoid conflicts!) The other patch of this bug should also be uplifted, otherwise this patch will negatively impact the data collected by one of our telemetry probes. [Is the change risky?]: no [Why is the change risky/not risky?]: Small javascript fix, only impacting Devtools GCLI users. [String changes made/needed]: none
Flags: needinfo?(jdescottes)
Attachment #8893233 -
Flags: approval-mozilla-beta?
Comment 22•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)
Reporter | ||
Comment 23•7 years ago
|
||
(In reply to Gerry Chang [:gchang] from comment #22) > Hi Brindusa, could you help find someone to verify if this issue was fixed > as expected on the latest Nightly build? Thanks! FWIW I can verify that I am no longer seeing the issue in today's (2017-08-14) nightly.
Comment 24•7 years ago
|
||
Comment on attachment 8893233 [details] Bug 1386821 - call initDevTools if devtools.toolbar.visible is true; We want dev tools users to consistently see the toolbar. Fix verified, let's uplift for beta 3.
Attachment #8893233 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 25•7 years ago
|
||
Comment on attachment 8893340 [details] Bug 1386821 - add a separate flag to check if devtools entry point was recorded; Note, we need the patch in bug 1387359 to land first.
Attachment #8893340 -
Flags: approval-mozilla-beta+
Assignee | ||
Comment 27•7 years ago
|
||
This patch is rebased on the latest beta and merges both patches that landed on central. I can't build beta locally for some reason, so let's wait for try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a2d1f426590c2e4b1ea4b7a60df1179021baae If try is green I will request for uplift again.
Flags: needinfo?(jdescottes)
Attachment #8897774 -
Flags: review+
Assignee | ||
Comment 28•7 years ago
|
||
Try looks green except for unrelated intermittents. Can we use attachment 8897774 [details] [diff] [review] for the beta uplift?
Flags: needinfo?(ryanvm)
Updated•7 years ago
|
Flags: needinfo?(ryanvm)
Comment 29•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c105be29eb27
Updated•7 years ago
|
Flags: qe-verify+
Comment 30•7 years ago
|
||
I managed to reproduce the issue on an older Nightly 57.0a1 (2017-08-03) build under Windows 10 x64. Verified as fixed using latest Nightly 57.0a1 (2017-08-17) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.12.6.
Comment 31•7 years ago
|
||
I also verified that this is fixed on Firefox 56 beta 9 using Windows 10 x64 and Ubuntu 16.04 x64.
Flags: qe-verify+
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•