loadURI function should call openLinkIn

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dao, Assigned: lviknesh, Mentored)

Tracking

Trunk
Firefox 37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

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,
  ...
});
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.
Posted patch loadURI.patch (obsolete) — Splinter Review
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
Attachment #8460212 - Flags: feedback?(dao)
Flags: needinfo?(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.
Attachment #8460212 - Flags: feedback+
Flags: needinfo?(manishearth)
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.
Attachment #8460212 - Flags: feedback?(dao) → feedback+
Posted patch load-uri.patch (obsolete) — Splinter Review
Attachment #8460212 - Attachment is obsolete: true
Attachment #8461013 - Flags: review?(dao)
Flags: needinfo?(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
Attachment #8461013 - Flags: feedback-
Flags: needinfo?(manishearth)
Comment on attachment 8461013 [details] [diff] [review]
load-uri.patch

Manish is right...
Attachment #8461013 - Flags: review?(dao)
Posted patch openLinkIn.patch (obsolete) — Splinter Review
Attachment #8461013 - Attachment is obsolete: true
Attachment #8462583 - Flags: feedback?(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(...)
Attachment #8462583 - Flags: feedback?(manishearth) → feedback-
Posted patch openLinkIn.patch (obsolete) — Splinter Review
Attachment #8462583 - Attachment is obsolete: true
Attachment #8462618 - Flags: feedback?(manishearth)
Comment on attachment 8462618 [details] [diff] [review]
openLinkIn.patch

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

LGTM
Attachment #8462618 - Flags: review?(dao)
Attachment #8462618 - Flags: feedback?(manishearth)
Attachment #8462618 - Flags: feedback+
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!
Attachment #8462618 - Flags: review?(dao) → review+
Posted patch final.patch (obsolete) — Splinter Review
Attachment #8462618 - Attachment is obsolete: true
Attachment #8463012 - Flags: review?(dao)
Comment on attachment 8463012 [details] [diff] [review]
final.patch

thanks!
Attachment #8463012 - Flags: review?(dao) → review+
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
(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?
Flags: needinfo?(lviknesh)
Hi Dao,

Sorry for delaying this . Corrected the typo :) Thanks :)
Attachment #8463012 - Attachment is obsolete: true
Flags: needinfo?(lviknesh)
Attachment #8536192 - Flags: feedback?(dao)
Comment on attachment 8536192 [details] [diff] [review]
typo-corrected

Thanks

https://hg.mozilla.org/integration/fx-team/rev/012389891b52
Attachment #8536192 - Flags: feedback?(dao) → feedback+
https://hg.mozilla.org/mozilla-central/rev/012389891b52
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.