728749 - avoid using nsIURILoader in contentAreaUtils.js openURL, when possible

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: patch

In the quite common case that we have a navigator:browser window, we should use the switchToTabHavingURI function to avoid running the rest of the code in the function, which is often slow and makes the browser look unresponsive.

If we don't have a navigator:browser window, or it doesn't have the function, fall back to the current behaviour.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

switchToTabHavingURI's behavior of switching to an already-loaded tab if it exists doesn't seem desirable for this function.

What is slow about the URILoader call?

Comment 2

[Geoff Lankow (:darktrojan)]

Dunno, but sometimes the 'check if your plugins are up to date' button in the addon manager has taken seconds to do anything.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 598736 [details] [diff] [review]
patch

r- for the first part of comment 1.

The link in the addons manager seems relatively snappy for me (though it may have been slower the first time).

A better shortcut for this would probably be to use recentWindow.browserDOMWindow.openURI(uri, null, OPEN_NEWTAB, 0), and then call focus() on the returned window.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> The link in the addons manager seems relatively snappy for me (though it may
> have been slower the first time).

It's extremely slow for me (sometimes takes ~10 seconds to open the tab).

Comment 5

[Marco Castelluccio [:marco]]

I've sent the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=fce464dcb502

Comment 6

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> What is slow about the URILoader call?

I've filed bug 753548.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 621996 [details] [diff] [review]
Patch v2

As discussed on IRC, I think we need to investigate why the other code path is so slow. It doesn't need to block landing this, but I think it's quite important that we not just wallpaper over this and then forget about the underlying issue.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> As discussed on IRC, I think we need to investigate why the other code path
> is so slow. It doesn't need to block landing this, but I think it's quite
> important that we not just wallpaper over this and then forget about the
> underlying issue.

FTR, the other code path is so slow because that's how the nsBrowserContentHandler works. The nsIURILoader asynchronously opens a channel that starts a request for the given URI. When the first bytes of data are received the request gets cancelled and the tab is opened. This can of course take some time on a slow network, with a slow DNS or the like.

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#707

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 621996 [details] [diff] [review]
Patch v2

Review of attachment 621996 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/contentAreaUtils.js
@@ +1100,5 @@
> +    var wmSvc = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +                          .getService(Components.interfaces.nsIWindowMediator);
> +    var recentWindow = wmSvc.getMostRecentWindow("navigator:browser");
> +    if (recentWindow) {
> +      var win = recentWindow.browserDOMWindow.openURI(uri, null, recentWindow.browserDOMWindow.OPEN_NEWTAB, 0);

This is ignoring the users tab preferences, using OPEN_DEFAULTWINDOW would make it respect the prefs correctly I think. The last argument should be OPEN_NEW then.

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> FTR, the other code path is so slow because ...

Bug 753548

(In reply to Dave Townsend (:Mossop) from comment #9)
> This is ignoring the users tab preferences, using OPEN_DEFAULTWINDOW would
> make it respect the prefs correctly I think. The last argument should be
> OPEN_NEW then.

Thank you, I'll try

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 623320 [details] [diff] [review]
Patch v3

I'm not sure it really makes sense to obey these user preferences, but I suppose it doesn't much matter.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2fdd0dbb97a

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b2fdd0dbb97a

Comment 15

[[e-mail]]

(In reply to Dave Townsend from comment #9)
> This is ignoring the users tab preferences

(In reply to Gavin Sharp from comment #12)
> I'm not sure it really makes sense to obey these user preferences, but I
> suppose it doesn't much matter.

Bug 914748 switched back to always opening in tabs anyway. (I don't know of a better API to open links, nor do I know why Firefox's nsBrowserAccess failed in the given case.)