1514632 - Load chat preview and cloudfile account settings only if needed

Status: RESOLVED FIXED

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1514632-preferences-browsers-1.diff

Comment 2

[Jorg K (GMT+2)]

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.

Comment 3

[Geoff Lankow (:darktrojan)]

Because after bug 1515174 it's not needed. Is that the only reason you didn't r+ this?

Comment 4

[Jorg K (GMT+2)]

I didn't look at it since I need a new patch with the flex=1 removed.

Comment 5

[Jorg K (GMT+2)]

And frankly, I'm not the right reviewer for this. Magnus or Aceman would be better, but there you don't get fast service :-(

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1514632-preferences-browsers-2.diff

Comment 7

[:aceman]

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()

Comment 8

[Geoff Lankow (:darktrojan)]

(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.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1514632-preferences-browsers-3.diff

Comment 10

[:aceman]

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...

Comment 11

[Geoff Lankow (:darktrojan)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1025d59f7259
Load chat preview and cloudfile account settings only if needed; r=aceman