Update object property names that get passed to loadURIOptions to match the names in loadURIOptions
Categories
(Firefox :: General, enhancement, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: ckerschb, Assigned: smurfd, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js][lang=cpp])
Attachments
(2 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
As a follow up to Bug 1513241, comment 27 [1] it makes sense to update the object property names that get passed into loadURIOptions to match the names within loadURIOptions.webidl.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1513241#c27
Reporter | ||
Comment 1•6 years ago
|
||
For the reference, here is a list of the dictionary names:
Principal? triggeringPrincipal = null;
long loadFlags = 0;
URI? referrerURI = null;
long referrerPolicy = 0;
InputStream? postData = null;
InputStream? headers = null;
URI? baseURI = null;
Please also note that we update C++ code to use postData rather than postStream, see also [1]. Finally within bug 1518454 we will update the dicationary to also include
nsIContentSecurityPolicy csp = null;
I think it makes sense to update frontend code to match those names so naming is used consistently thoughout the various layers of API.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1513241#c28
Reporter | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
Gijs, are you interested into mentoring this bug?
Comment 3•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #2)
Gijs, are you interested into mentoring this bug?
Hm. This will involve quite a bit of tedious work making sure that parameter names are correctly updated, but will ultimately make the codebase cleaner as a result. At the moment, this would involve making any places where we refer to just aFlags
or flags
use the loadFlags
variant, and making places where we use postStream
consistently use postData
. Given it's repetitive, that makes it plausible as a mentored bug, but all of these cases are pretty finnicky... we can give it a try, but it's not good first bug material, more like good third or fourth bug, and at that point I dunno if it makes sense to treat it as a mentored bug.
Comment 4•6 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1517703 looks like it'll generate more work to do here.
Assignee | ||
Comment 5•6 years ago
|
||
Hey, i could be interested in looking at this, if i understand things correctly :)
I did something kind of similar in Bug 1122740 & 1515419.
If i understand correctly we are talking about these things :
-
We want to change that anytime loadURIOptions is used, and parameters passed to it, those parameter names should match what's in dom/webidl/LoadURIOptions.webidl?
-
We also want to change that whenever 'postStream' is used, it should be replaced by 'postData'.
Even in the webidl file https://searchfox.org/mozilla-central/source/dom/webidl/LoadURIOptions.webidl#48 ? -
We also want to change that whenever 'flags' or 'aFlags' is used, it should be replaced by 'loadFlags' ?
What im not sure of, 2 & 3 is going through the whole codebase or "just" related to loadURIOptions?
so something like this. Should it be changed to loadFlags or not?
https://searchfox.org/mozilla-central/source/gfx/2d/2D.h#1781
Comment 6•6 years ago
|
||
(In reply to Nicklas Boman [:smurfd] from comment #5)
Hey, i could be interested in looking at this, if i understand things correctly :)
I did something kind of similar in Bug 1122740 & 1515419.
If i understand correctly we are talking about these things :
- We want to change that anytime loadURIOptions is used, and parameters passed to it, those parameter names should match what's in dom/webidl/LoadURIOptions.webidl?
Yes.
- We also want to change that whenever 'postStream' is used, it should be replaced by 'postData'.
Even in the webidl file https://searchfox.org/mozilla-central/source/dom/webidl/LoadURIOptions.webidl#48 ?
Yes, in relation to loadURIOptions and nsDocShell.cpp/h and friends.
- We also want to change that whenever 'flags' or 'aFlags' is used, it should be replaced by 'loadFlags' ?
Right, but only in relation to loadURIOptions
What im not sure of, 2 & 3 is going through the whole codebase or "just" related to loadURIOptions?
so something like this. Should it be changed to loadFlags or not?
https://searchfox.org/mozilla-central/source/gfx/2d/2D.h#1781
No, we shouldn't touch 'aFlags' everywhere else.
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
So, cough it took some time before i actually started to look at it.
I have looked for loadURIOptions via Searchfox and there where only a few places in C code that the properties passed into it, was different than the dictionarynames.
Guess things has changed some since bug1517703 landed.
Or am i missing a ton? please let me know :)
Where i to go through the JavaScript code aswell?
Assignee | ||
Comment 10•6 years ago
|
||
Im assuming a line like https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1237 should be changed to : loadFlags: loadFlags,
If so the attached patch could be on a good way...
Im worried that i have not gone deep enough, since i have not changed That much...
Comment 11•6 years ago
|
||
(In reply to Nicklas Boman [:smurfd] from comment #10)
Created attachment 9067866 [details] [diff] [review]
bug1519365-190527-2050.patchIm assuming a line like https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1237 should be changed to : loadFlags: loadFlags,
Not quite, it can just be
loadFlags,
If so the attached patch could be on a good way...
Im worried that i have not gone deep enough, since i have not changed That much...
Off-hand it looks like you're on the right track. Please upload to phabricator though, as it'll make reviewing subsequent iterations of the patch easier. :-)
Thanks for working on this!
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Can a follow up be filed to only support browser.loadURI
with loadFlags
and not flags
? This commit might be useful as list of browser.loadURI
usages: https://hg.mozilla.org/mozilla-central/rev/14a3a7e06735
Comment 13•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #12)
Can a follow up be filed to only support
browser.loadURI
withloadFlags
and notflags
? This commit might be useful as list ofbrowser.loadURI
usages: https://hg.mozilla.org/mozilla-central/rev/14a3a7e06735
Yes, I'll do that once this lands.
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
bugherder |
Description
•