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)

defect

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)

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
[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?
Flags: needinfo?(rchien)
Priority: -- → P2
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
Status: NEW → ASSIGNED
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?
Nice catch! I've moved entire `promiseLoadHandlersList` section to preferences.js to make sure the callback will get initialized at the beginning.
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 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+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c9a3206fa9f
Initialize Applications pane in preferences.js r=jaws
Priority: P2 → P3
Whiteboard: [photon-preference]
https://hg.mozilla.org/mozilla-central/rev/4c9a3206fa9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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)
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?
Status: RESOLVED → VERIFIED
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+
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+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: