Rename `flags` to `loadFlags` in the options object argument passed by browser.loadURI consumers
Categories
(Firefox :: General, task, P5)
Tracking
()
People
(Reporter: Gijs, Unassigned, Mentored)
References
Details
After bug 1519365 we accept both variations. We should update consumers so we can stop supporting the flags option and so it's easier to find loadFlags consumers in the tree, and there's less confusion about all the different meanings flags can have.
This is a mentored bug, but probably not the best thing for a "good first bug", because of how many places need updating. It's probably a good idea to do per-toplevel-directory patches for browser/ and toolkit/ and then a third for "everything else".
|
||
Comment 1•5 years ago
|
||
|
||
Updated•5 years ago
|
I'd like to help with this. Just give me a rundown of what must be done.
|
Reporter | |
Comment 3•5 years ago
|
||
(In reply to premk from comment #2)
I'd like to help with this. Just give me a rundown of what must be done.
There's a high-level summary in comment #0. Basically, go through the codebase (probably using searchfox) for places that call the loadURI method on a browser object with an options argument that contains a flags property, and make it use a loadFlags property instead. Happy to help clarify that further if you have a specific question.
So I just look for all instances of browser.loadURI method call in searchfox and replace the flags property with loadFlags ?
https://searchfox.org/mozilla-central/search?q=browser.loaduri&path=
I'm seeing other objects gbrowser that use the loadURI method with the flags property. Should I ignore those?
https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#3297
|
|
Reporter | |
Comment 5•5 years ago
|
||
(In reply to premk from comment #4)
So I just look for all instances of
browser.loadURImethod call in searchfox and replace theflagsproperty withloadFlags?
We can't assume that the variable referring to the browser is called browser, so we'll probably have to look at instances of .loadURI, and then read the surrounding code to work out if we're passing an options arg with a flags property, and if this is a browser object or not.
https://searchfox.org/mozilla-central/search?q=browser.loaduri&path=
I'm seeing other objects
gbrowserthat use theloadURImethod with the flags property. Should I ignore those?https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#3297
We'll have to look on a case-by-case basis. In this case, gBrowser is implemented in tabbrowser.js, so if you look at https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/browser/base/content/tabbrowser.js#427-429 you'll see that this just forwards to the selected browser, so its callers will also need updating.
(In reply to :Gijs (he/him) from comment #5)
We can't assume that the variable referring to the browser is called
browser, so we'll probably have to look at instances of.loadURI, and then read the surrounding code to work out if we're passing an options arg with aflagsproperty, and if this is a browser object or not.
Got it. Just looked it up; there are quite a few instance of the .loadURI method in the code base.
I will use Comment #12 in Bug 1519365 as a starting point and work from there.
|
|
||
Comment 7•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
|
||
Updated•2 years ago
|
(In reply to :Gijs (he/him) from comment #0)
probably not the best thing for a "good first bug", because of how many places need updating. It's probably a good idea to do per-toplevel-directory patches for browser/ and toolkit/ and then a third for "everything else".
Hi, I'd like to give this one a try and I did search for all .loadURI instances on searchfox, but from what I can see there are only 4 places where an args with a flag property is being passed. However you clearly mentioned that there are a lot of places that need updating, am I doing this wrong? Or has the code been updated majorly seeing as this bug was filed 4 years ago?
|
|
Reporter | |
Comment 9•1 year ago
|
||
The arguments get passed around a lot, which is where the other places can be found, but at this point this bug is basically equivalent to bug 1558149 so I'll mark it as a duplicate. Sorry for the confusion!
Description
•