Closed
Bug 1444760
Opened 7 years ago
Closed 7 years ago
Combine loadURIWithFlags with loadURI
Categories
(Firefox :: Tabbed Browser, task)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ntim, Assigned: ntim)
References
Details
Attachments
(2 files)
Right now, two forms of loadURIWithFlags are supported:
loadURIWithFlags(aURI, aFlags, aReferrerURI, aCharset, aPostData);
and
loadURIWithFlags(aURI, aParams);
where aParams is an object containing aFlags, aReferrerURI, aCharset, aPostData.
This bug is about supporting only the second form.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
I'm thinking it would be possible even to combine loadURI and loadURIWithFlags into one single method since loadURI is basically loadURIWithFlags with a reduced number of parameters. We could do that really easily with ES6 default parameters.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Summary: Support only one form of loadURIWithFlags calls → Combine loadURIWithFlags with loadURI
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
New Part 1 & 2 try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8067c6c3ff96252982af5b4ff52d33783093c782
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 15•7 years ago
|
||
Updated•7 years ago
|
Blocks: war-on-xbl
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8957960 [details]
Bug 1444760 - Support only one form of loadURIWithFlags calls.
https://reviewboard.mozilla.org/r/226876/#review232762
::: browser/base/content/browser.js:5367
(Diff revision 5)
> if (aURI) {
> let loadflags = isExternal ?
> Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL :
> Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> gBrowser.loadURIWithFlags(aURI.spec, {
> - aTriggeringPrincipal,
> + aTriggeringPrincipal,
triggeringPrincipal: aTriggeringPrincipal,
::: browser/base/content/tabbrowser.js
(Diff revision 5)
> aIID.equals(Ci.nsISupports))
> return this;
> throw Cr.NS_NOINTERFACE;
> }
> }
> -
please drop this change
Attachment #8957960 -
Flags: review?(dao+bmo) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8957963 [details]
Bug 1444760 - Combine loadURIWithFlags and loadURI methods.
https://reviewboard.mozilla.org/r/226878/#review234146
Nice cleanup, thanks!
Attachment #8957963 -
Flags: review?(dao+bmo) → review+
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #16)
> ::: browser/base/content/tabbrowser.js
> (Diff revision 5)
> > aIID.equals(Ci.nsISupports))
> > return this;
> > throw Cr.NS_NOINTERFACE;
> > }
> > }
> > -
>
> please drop this change
Looks like there was 2 empty lines at the end of the file before the change, and Atom made it so there's only one empty line left at the end of the file. Seems like a reasonable change to me, but I'm happy to fight Atom with adding back the second empty line if needed :)
Comment 23•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 73f75121ac28326aa83e5dfc2637dede2558c47d -d 6426e089e5c5: rebasing 452734:73f75121ac28 "Bug 1444760 - Support only one form of loadURIWithFlags calls. r=dao"
merging browser/base/content/browser.js
merging browser/base/content/tabbrowser.js
merging browser/base/content/tabbrowser.xml
merging toolkit/content/widgets/browser.xml
merging toolkit/mozapps/extensions/content/extensions.js
rebasing 452735:aaee500f8c9c "Bug 1444760 - Combine loadURIWithFlags and loadURI methods. r=dao" (tip)
merging browser/base/content/browser.js
merging browser/base/content/tabbrowser.js
merging browser/base/content/tabbrowser.xml
merging browser/components/extensions/ext-devtools-panels.js
merging browser/components/extensions/ext-devtools.js
merging browser/components/search/test/browser_aboutSearchReset.js
merging browser/components/sessionstore/SessionStore.jsm
merging toolkit/content/widgets/browser.xml
merging toolkit/mozapps/extensions/content/extensions.js
warning: conflicts while merging browser/base/content/tabbrowser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment 24•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c52361ab381
Support only one form of loadURIWithFlags calls. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e0ecafa703
Combine loadURIWithFlags and loadURI methods. r=dao
Comment 25•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e77f90bdb4
Followup: Fix broken rebase in tabbrowser.js. r=me
Comment 26•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b40369822f
Followup: Fix params is null exception. r=me
Comment 27•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00da6f45a098
Followup: Fix 'params is undefined' exception on a CLOSED TREE. r=me
Comment 28•7 years ago
|
||
Backed out for failing talos regarging undefined params
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=00da6f45a098a51748cce5759fb3ed6241d6afa9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168729641&repo=mozilla-inbound&lineNumber=12217
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/384c57a10906f919aa27af99575186e9f14aaa20
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8957960 [details]
Bug 1444760 - Support only one form of loadURIWithFlags calls.
https://reviewboard.mozilla.org/r/226876/#review236422
Code analysis found 1 defect in this patch:
- 1 defect found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:57
(Diff revision 9)
> break;
> case "GeckoView:GoForward":
> this.browser.goForward();
> break;
> case "GeckoView:LoadUri":
> const { uri, referrer, baseUri, flags } = aData;
Error: 'baseuri' is assigned a value but never used. [eslint: no-unused-vars]
Comment 34•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4956db1e9bdd
Support only one form of loadURIWithFlags calls. r=dao
https://hg.mozilla.org/integration/autoland/rev/71f61091a716
Combine loadURIWithFlags and loadURI methods. r=dao
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8957963 [details]
Bug 1444760 - Combine loadURIWithFlags and loadURI methods.
https://reviewboard.mozilla.org/r/226878/#review236424
Code analysis found 2 defects in this patch:
- 2 defects found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: browser/base/content/browser.js:1136
(Diff revision 10)
> if (mustChangeProcess) {
> Cu.reportError(e);
> gBrowser.updateBrowserRemotenessByURL(browser, uri);
>
> - if (params.userContextId) {
> - browser.webNavigation.setOriginAttributesBeforeLoading({ userContextId: params.userContextId });
> + if (userContextId) {
> + browser.webNavigation.setOriginAttributesBeforeLoading({ userContextId: userContextId });
Error: Expected property shorthand. [eslint: object-shorthand]
::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:57
(Diff revision 10)
> break;
> case "GeckoView:GoForward":
> this.browser.goForward();
> break;
> case "GeckoView:LoadUri":
> const { uri, referrer, baseUri, flags } = aData;
Error: 'baseuri' is assigned a value but never used. [eslint: no-unused-vars]
Comment 36•7 years ago
|
||
Backed out 2 changesets (bug 1444760) for ES lint failure in /builds/worker/checkouts/gecko/browser/base/content/browser.js
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=71f61091a7169af47af720db4930d4243ebe4900&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=170134049
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170134049&repo=autoland&lineNumber=261
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=de459b5357bb2bf6b3f83de6b462cf3800d813c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cedad7dd5d15
Support only one form of loadURIWithFlags calls. r=dao
https://hg.mozilla.org/integration/autoland/rev/14a3a7e06735
Combine loadURIWithFlags and loadURI methods. r=dao
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cedad7dd5d15
https://hg.mozilla.org/mozilla-central/rev/14a3a7e06735
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Updated•6 years ago
|
Type: defect → task
You need to log in
before you can comment on or make changes to this bug.
Description
•