avoid using nsIURILoader in contentAreaUtils.js openURL, when possible

RESOLVED FIXED in mozilla15

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: marco)

Tracking

unspecified
mozilla15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p2])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 598736 [details] [diff] [review]
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.
Attachment #598736 - Flags: review?(gavin.sharp)
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?
(Reporter)

Comment 2

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

Updated

7 years ago
Whiteboard: [snappy] → [snappy:p2]
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.
Attachment #598736 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 4

7 years ago
Created attachment 621996 [details] [diff] [review]
Patch v2

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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).
Attachment #621996 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

7 years ago
I've sent the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=fce464dcb502
(Assignee)

Comment 6

7 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> What is slow about the URILoader call?

I've filed bug 753548.
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.
Attachment #621996 - Flags: review?(gavin.sharp) → review+
Summary: Use switchToTabHavingURI in contentAreaUtils.js openURL, where available → avoid using nsIURILoader in contentAreaUtils.js openURL, when possible
Attachment #598736 - Attachment is obsolete: true
Assignee: geoff → mar.castelluccio
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 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.
(Assignee)

Comment 10

7 years ago
(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
Keywords: checkin-needed
(Assignee)

Comment 11

7 years ago
Created attachment 623320 [details] [diff] [review]
Patch v3
Attachment #621996 - Attachment is obsolete: true
Attachment #623320 - Flags: review?(dtownsend+bugmail)
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.
Attachment #623320 - Flags: review?(dtownsend+bugmail) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2fdd0dbb97a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b2fdd0dbb97a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 914748
(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.)
You need to log in before you can comment on or make changes to this bug.