1038599 - loadURI function should call openLinkIn

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [::dao]]

loadURI is implemented here:

http://hg.mozilla.org/mozilla-central/annotate/095d2a9c2be5/browser/base/content/browser.js#l1900

This can be simplified by calling openLinkIn from within this function, e.g.:

openLinkIn(uri, "current", {
  referrerURI: referrer,
  ...
});

Comment 1

[Manish Goregaokar [:manishearth]]

Note: Source of openLinkIn is http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#207, in case you want to understand what the parameters do.

Comment 2

[vikneshwar]

Attachment: loadURI.patch

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8460212 [details] [diff] [review]
loadURI.patch

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

Looks good, but I'm not sure if the postData bit is necessary or not.

Comment 4

[Dão Gottwald [::dao]]

Comment on attachment 8460212 [details] [diff] [review]
loadURI.patch

>   if (postData === undefined)
>     postData = null;

This should be removed.

>-  var flags = nsIWebNavigation.LOAD_FLAGS_NONE;
>-  if (allowThirdPartyFixup) {
>-    flags |= nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>-    flags |= nsIWebNavigation.LOAD_FLAGS_FIXUP_SCHEME_TYPOS;
>-  }
>-
>-  try {
>-    gBrowser.loadURIWithFlags(uri, flags, referrer, null, postData);
>-  } catch (e) {}
>+  openLinkIn(uri, "current",
>+             { referrerURI: referrer,
>+               postData: postData,
>+               allowThirdPartyFixup: allowThirdPartyFixup });

try/catch should be kept to maintain current behavior. An exception would be thrown for unknown protocols.

Comment 5

[vikneshwar]

Attachment: load-uri.patch

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8461013 [details] [diff] [review]
load-uri.patch

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

In the future, feedback? is a better flag to use over needinfo :)

::: browser/base/content/browser.js
@@ +2009,5 @@
>  }
>  
>  function loadURI(uri, referrer, postData, allowThirdPartyFixup) {
> +
> +  openLinkIn(uri, "current",

You're calling openLinkIn *and* loadURIWithFlags here, that's wrong.

We want to replace the loadURIWithFlags called with an openLinkIn call

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 8461013 [details] [diff] [review]
load-uri.patch

Manish is right...

Comment 8

[vikneshwar]

Attachment: openLinkIn.patch

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8462583 [details] [diff] [review]
openLinkIn.patch

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

::: browser/base/content/browser.js
@@ +2009,5 @@
>  }
>  
>  function loadURI(uri, referrer, postData, allowThirdPartyFixup) {
> +
> +  openLinkIn(uri, "current",

You're still doing it twice.

This bit over here is unnecessary.

@@ +2017,3 @@
>  
>    try {
> +    gBrowser.openLinkIn(uri, "current",

Check if it's gBrowser.openLinkIn(...) or just openLinkIn(...)

Comment 10

[vikneshwar]

Attachment: openLinkIn.patch

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8462618 [details] [diff] [review]
openLinkIn.patch

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

LGTM

Comment 12

[Dão Gottwald [::dao]]

Comment on attachment 8462618 [details] [diff] [review]
openLinkIn.patch

> function loadURI(uri, referrer, postData, allowThirdPartyFixup) {
>-  if (postData === undefined)
>-    postData = null;
>-
>-  var flags = nsIWebNavigation.LOAD_FLAGS_NONE;
>-  if (allowThirdPartyFixup) {
>-    flags |= nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>-    flags |= nsIWebNavigation.LOAD_FLAGS_FIXUP_SCHEME_TYPOS;
>-  }
> 
>   try {

Please remove the blank line before try.

>-    gBrowser.loadURIWithFlags(uri, flags, referrer, null, postData);
>+    openLinkIn(uri, "current",
>+               { referrerURI: referrer,
>+                 postData: postData,
>+                 allowThirdPartyFixup: allowThirdPartyFixUp,
>+                 charset: null });

Please remove the charset parameter. It's optional, so there's no need to set it to null.

Looks good otherwise. Thanks!

Comment 13

[vikneshwar]

Attachment: final.patch

Comment 14

[Dão Gottwald [::dao]]

Comment on attachment 8463012 [details] [diff] [review]
final.patch

thanks!

Comment 15

[Dão Gottwald [::dao]]

https://hg.mozilla.org/integration/fx-team/rev/29b357a370f3

Comment 16

[Dão Gottwald [::dao]]

Backed out (https://hg.mozilla.org/integration/fx-team/rev/bc751d1d8c1d) to see if this was responsible for test failures (https://tbpl.mozilla.org/?tree=Fx-Team&rev=29b357a370f3).

Comment 17

[Dão Gottwald [::dao]]

test failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js | Test timed out

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_ContentSearch.js | Test timed out

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/states/test_link.html | Test timed out

Comment 18

[Dão Gottwald [::dao]]

(In reply to Dão Gottwald [:dao] from comment #17)
> test failures:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_aboutHome.js | Test timed out
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/modules/test/
> browser_ContentSearch.js | Test timed out
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/tests/mochitest/states/test_link.
> html | Test timed out

Looks like this was due to a typo in the patch: allowThirdPartyFixup vs. allowThirdPartyFixUp

Vikneshwar, would you mind updating the patch?

Comment 19

[vikneshwar]

Attachment: typo-corrected

Hi Dao,

Sorry for delaying this . Corrected the typo :) Thanks :)

Comment 20

[Dão Gottwald [::dao]]

Comment on attachment 8536192 [details] [diff] [review]
typo-corrected

Thanks

https://hg.mozilla.org/integration/fx-team/rev/012389891b52

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/012389891b52