Closed
Bug 1514632
Opened 4 years ago
Closed 3 years ago
Load chat preview and cloudfile account settings only if needed
Categories
(Thunderbird :: Preferences, enhancement)
Thunderbird
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 66.0
People
(Reporter: darktrojan, Assigned: darktrojan)
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
13.67 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
The chat preview loads a document in a XUL browser and cloudfile settings loads a document in an iframe, but neither of them will be used in most visits to the preferences tab. In fact, both features can be disabled by prefs but their documents still load. We should delay doing this until needed.
Assignee | ||
Comment 1•4 years ago
|
||
Attachment #9032066 -
Flags: review?(jorgk)
Comment 2•4 years ago
|
||
Comment on attachment 9032066 [details] [diff] [review] 1514632-preferences-browsers-1.diff Review of attachment 9032066 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/chat.inc.xul @@ +1,4 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > + <prefpane id="paneChat" label="&paneChat.title;" onpaneload="gChatPane.init();" flex="1"> You wanted to remove this again in bug 1515174.
Attachment #9032066 -
Flags: review?(jorgk)
Assignee | ||
Comment 3•4 years ago
|
||
Because after bug 1515174 it's not needed. Is that the only reason you didn't r+ this?
Comment 4•4 years ago
|
||
I didn't look at it since I need a new patch with the flex=1 removed.
Comment 5•4 years ago
|
||
And frankly, I'm not the right reviewer for this. Magnus or Aceman would be better, but there you don't get fast service :-(
Assignee | ||
Comment 6•4 years ago
|
||
Attachment #9032066 -
Attachment is obsolete: true
Attachment #9032538 -
Flags: review?(acelists)
Comment on attachment 9032538 [details] [diff] [review] 1514632-preferences-browsers-2.diff Review of attachment 9032538 [details] [diff] [review]: ----------------------------------------------------------------- Yes, but how it complicates the code :( Can't it be made simpler? ::: mail/components/preferences/chat.inc.xul @@ +6,5 @@ > <script type="application/javascript" src="chrome://messenger/content/preferences/messagestyle.js"/> > > <preferences id="chatPreferences"> > + <preference id="mail.preferences.chat.selectedTabIndex" > + name="mail.preferences.chat.selectedTabIndex" type="int"/> Why is this needed? How does it relate to the late loading of the browser? ::: mail/components/preferences/chat.js @@ +14,5 @@ > this.updatePlaySound(); > + > + this.mTabBox = document.getElementById("chatPrefs"); > + if (!(("arguments" in window) && window.arguments[1])) { > + // If no tab was specified, select the last used tab. And what if a tab was specified? But why do we need to handle restoring selected tab here? @@ +19,5 @@ > + let preference = document.getElementById("mail.preferences.chat.selectedTabIndex"); > + this.mTabBox.selectedIndex = preference.value != null ? preference.value : 0; > + } > + > + this.paneSelectionChanged = this.paneSelectionChanged.bind(this); What? @@ +46,5 @@ > + return; > + } > + if (getCurrentPaneID() != "paneChat") { > + return; > + } This check could be in paneSelectionChanged()
Assignee | ||
Comment 8•3 years ago
|
||
(In reply to :aceman from comment #7) > Why is this needed? How does it relate to the late loading of the browser? I should've said, that I also made the chat pane remember which tab it had selected. The other panes with tabs do this, and I nicked the code from one of them. I'm pretty sure the bits about window.arguments are unused now we load prefs in a tab, so I'll get rid of them. > @@ +19,5 @@ > > + let preference = document.getElementById("mail.preferences.chat.selectedTabIndex"); > > + this.mTabBox.selectedIndex = preference.value != null ? preference.value : 0; > > + } > > + > > + this.paneSelectionChanged = this.paneSelectionChanged.bind(this); > > What? We need to make sure that when paneSelectionChanged is called, that this is gChatPane, not window. > @@ +46,5 @@ > > + return; > > + } > > + if (getCurrentPaneID() != "paneChat") { > > + return; > > + } > > This check could be in paneSelectionChanged() It'd need to be in tabSelectionChanged too, since that is called when the tab is opened, so it might as well be where it is.
Assignee | ||
Comment 9•3 years ago
|
||
Attachment #9032538 -
Attachment is obsolete: true
Attachment #9032538 -
Flags: review?(acelists)
Attachment #9033309 -
Flags: review?(acelists)
![]() |
||
Comment 10•3 years ago
|
||
Comment on attachment 9033309 [details] [diff] [review] 1514632-preferences-browsers-3.diff Review of attachment 9033309 [details] [diff] [review]: ----------------------------------------------------------------- Still no simpler, but if it wins any speedup...
Attachment #9033309 -
Flags: review?(acelists) → review+
Assignee | ||
Comment 11•3 years ago
|
||
No simpler from a reading-the-code point of view, but when running the code it means doing one thing instead of three, which should also make debugging that tab less of a pain.
Comment 12•3 years ago
|
||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/1025d59f7259 Load chat preview and cloudfile account settings only if needed; r=aceman
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•3 years ago
|
Target Milestone: --- → Thunderbird 66.0
You need to log in
before you can comment on or make changes to this bug.
Description
•