Closed Bug 728749 Opened 9 years ago Closed 9 years ago

avoid using nsIURILoader in contentAreaUtils.js openURL, when possible

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: darktrojan, Assigned: marco)

References

Details

(Whiteboard: [snappy:p2])

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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?
Dunno, but sometimes the 'check if your plugins are up to date' button in the addon manager has taken seconds to do anything.
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-
Attached patch Patch v2 (obsolete) — Splinter Review
(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)
(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
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.
(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
Attached patch Patch v3Splinter Review
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+
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
Closed: 9 years ago
Resolution: --- → FIXED
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.