Closed Bug 1444760 Opened 2 years ago Closed 2 years ago

Combine loadURIWithFlags with loadURI

Categories

(Firefox :: Tabbed Browser, task)

task
Not set

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

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.
Depends on: 1442058
Blocks: 1442058
No longer depends on: 1442058
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.
Summary: Support only one form of loadURIWithFlags calls → Combine loadURIWithFlags with loadURI
Blocks: 1441935
No longer blocks: 1442058
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 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+
Blocks: 1387013
Status: NEW → ASSIGNED
(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 :)
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)
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
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e77f90bdb4
Followup: Fix broken rebase in tabbrowser.js. r=me
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b40369822f
Followup: Fix params is null exception. r=me
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 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]
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 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]
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
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/cedad7dd5d15
https://hg.mozilla.org/mozilla-central/rev/14a3a7e06735
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1442058
Type: defect → task
You need to log in before you can comment on or make changes to this bug.