Closed
Bug 1402918
Opened 7 years ago
Closed 7 years ago
Applications section is blank if preferences is launched not directly into General tab
Categories
(Firefox :: Settings UI, defect, P3)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | + | verified |
firefox58 | --- | verified |
People
(Reporter: itiel_yn8, Assigned: rickychien)
References
Details
(Keywords: regression, Whiteboard: [photon-preference])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
This one is a regression, I assume by one of the bugs under bug 1398597. STR: 1. Open a new tab 2. Type about:preferences#search and press enter (the same could be said about #sync, #containers etc., except #general) 3. Go to General tab, observe the Applications section AR: The Applications section is blank
Comment 1•7 years ago
|
||
[Tracking Requested - why for this release]: broken Applications section depending on how the Preferences is opened. https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a0eb21bf55e1c1ae0ba311e6f2273da05c712799&tochange=319a34bea9e4f3459886b5b9e835bd338320f1fd @ricky, could this be from your change to allow scrolling?
status-firefox57:
--- → affected
status-firefox58:
--- → affected
tracking-firefox57:
--- → ?
Flags: needinfo?(rchien)
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 2•7 years ago
|
||
No, it must be bug 1400117 since we only initialize the Application pane within pageshow event.
Assignee: nobody → rchien
Flags: needinfo?(rchien) → qe-verify+
QA Contact: hani.yacoub
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8912600 [details] Bug 1402918 - Initialize Applications pane in preferences.js https://reviewboard.mozilla.org/r/183930/#review189096 ::: browser/components/preferences/in-content/main.js:478 (Diff revision 1) > } > }, {once: true}); > }); > > + // Initialize Applications Pane in case the above pageshow callback doesn't trigger. > + this.initApplicationsPane(); How do you ensure this won't trigger main thread I/O before pageshow in the normal case?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Nice catch! I've moved entire `promiseLoadHandlersList` section to preferences.js to make sure the callback will get initialized at the beginning.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8912600 [details] Bug 1402918 - Initialize Applications pane in preferences.js https://reviewboard.mozilla.org/r/183930/#review189216 The changes I requested below aren't major, but I'd like to see it again before marking r+ ::: browser/components/preferences/in-content/main.js:133 (Diff revision 2) > _visibleTypeDescriptionCount: {}, > > > // Convenience & Performance Shortcuts > > // These get defined by init(). This comment is out of date now. ::: browser/components/preferences/in-content/main.js:135 (Diff revision 2) > - _prefsBundle: null, > - _list: null, > - _filter: null, > + this._bundleBrandShortName = this._bundleBrandShortName || > + document.getElementById("bundleBrand").getString("brandShortName"); > + return this._bundleBrandShortName; Instead of creating new member variables, you can do the following pattern: > get _brandShortName() { > delete this._brandShortName; > return this._brandShortName = document.getElementById("bundleBrand").getString("brandShortName"); > } and you can do this for the other getters that you've changed below. ::: browser/components/preferences/in-content/preferences.js:66 (Diff revision 2) > register_module("paneSearch", gSearchPane); > register_module("panePrivacy", gPrivacyPane); > register_module("paneContainers", gContainersPane); > register_module("paneSync", gSyncPane); > register_module("paneSearchResults", gSearchResultsPane); > gSearchResultsPane.init(); Please keep the promiseLoadHandlersList and the related Promise and event listener in main.js, then create a new gMainPane.preinit() function which will only set up the promiseLoadHandlersList promise. gMainPane.preinit() can be called after gSearchResultsPane.init()
Attachment #8912600 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8912600 [details] Bug 1402918 - Initialize Applications pane in preferences.js https://reviewboard.mozilla.org/r/183930/#review189276 r=me with the following issue fixed. ::: browser/components/preferences/in-content/main.js:137 (Diff revision 3) > - // These get defined by init(). > - _brandShortName: null, > + get _brandShortName() { > + return document.getElementById("bundleBrand").getString("brandShortName"); Can you delete these getters first? I had included that in the code snippet in the previous review. > get _brandShortName() { > delete this._brandShortName; > return this._brandShortName = document.getElementById("bundleBrand").getString("brandShortName"); > }
Attachment #8912600 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c9a3206fa9f Initialize Applications pane in preferences.js r=jaws
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P3
Whiteboard: [photon-preference]
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c9a3206fa9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 13•7 years ago
|
||
Build ID: 20170928100123 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Hi Ricky, should we uplift this fix to Beta57?
Flags: needinfo?(rchien)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8912600 [details] Bug 1402918 - Initialize Applications pane in preferences.js Approval Request Comment [Feature/Bug causing the regression]: bug 1400117 [User impact if declined]: usability issue. The Applications section could be blank [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: see description [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: normal [Why is the change risky/not risky?]: usability affect under certain condition [String changes made/needed]: none
Flags: needinfo?(rchien)
Attachment #8912600 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 16•7 years ago
|
||
Comment on attachment 8912600 [details] Bug 1402918 - Initialize Applications pane in preferences.js Fix a new regression, has been verified, taking it. Should be in 57b5
Attachment #8912600 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/216d7126983b
Comment 18•7 years ago
|
||
Build ID: 20171002181526 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Verified as fixed on Firefox Beta 57.0b5 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Flags: qe-verify+
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•