Closed
Bug 1038599
Opened 9 years ago
Closed 8 years ago
loadURI function should call openLinkIn
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 37
People
(Reporter: dao, Assigned: lviknesh, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 5 obsolete files)
1.28 KB,
patch
|
dao
:
feedback+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(manishearth)
Comment 3•9 years ago
|
||
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+
Updated•9 years ago
|
Flags: needinfo?(manishearth)
Reporter | ||
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8460212 -
Attachment is obsolete: true
Attachment #8461013 -
Flags: review?(dao)
Flags: needinfo?(manishearth)
Comment 6•9 years ago
|
||
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-
Updated•9 years ago
|
Flags: needinfo?(manishearth)
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8461013 [details] [diff] [review] load-uri.patch Manish is right...
Attachment #8461013 -
Flags: review?(dao)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8461013 -
Attachment is obsolete: true
Attachment #8462583 -
Flags: feedback?(manishearth)
Comment 9•9 years ago
|
||
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-
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8462583 -
Attachment is obsolete: true
Attachment #8462618 -
Flags: feedback?(manishearth)
Comment 11•9 years ago
|
||
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+
Reporter | ||
Comment 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8462618 -
Attachment is obsolete: true
Attachment #8463012 -
Flags: review?(dao)
Reporter | ||
Comment 14•9 years ago
|
||
Comment on attachment 8463012 [details] [diff] [review] final.patch thanks!
Attachment #8463012 -
Flags: review?(dao) → review+
Reporter | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/29b357a370f3
Reporter | ||
Comment 16•9 years ago
|
||
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).
Reporter | ||
Comment 17•9 years ago
|
||
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
Reporter | ||
Comment 18•8 years ago
|
||
(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)
Assignee | ||
Comment 19•8 years ago
|
||
Hi Dao, Sorry for delaying this . Corrected the typo :) Thanks :)
Attachment #8463012 -
Attachment is obsolete: true
Flags: needinfo?(lviknesh)
Attachment #8536192 -
Flags: feedback?(dao)
Reporter | ||
Comment 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/012389891b52
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in
before you can comment on or make changes to this bug.
Description
•