Load chat preview and cloudfile account settings only if needed

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({perf})

unspecified
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Attachment #9032066 - Flags: review?(jorgk)
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)
Because after bug 1515174 it's not needed. Is that the only reason you didn't r+ this?
I didn't look at it since I need a new patch with the flex=1 removed.
And frankly, I'm not the right reviewer for this. Magnus or Aceman would be better, but there you don't get fast service :-(
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()
(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.
Attachment #9032538 - Attachment is obsolete: true
Attachment #9032538 - Flags: review?(acelists)
Attachment #9033309 - Flags: review?(acelists)
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+
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.
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: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.