Closed Bug 1519365 Opened 6 years ago Closed 5 years ago

Update object property names that get passed to loadURIOptions to match the names in loadURIOptions

Categories

(Firefox :: General, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 69
Tracking Status
firefox69 --- fixed

People

(Reporter: ckerschb, Assigned: smurfd, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js][lang=cpp])

Attachments

(2 files)

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

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

Component: DOM: Security → General
Product: Core → Firefox

Gijs, are you interested into mentoring this bug?

Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P5

(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.

Mentor: gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [lang=js][lang=cpp]

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 :

  1. 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?

  2. 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 ?

  3. 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

Flags: needinfo?(gijskruitbosch+bugs)

(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 :

  1. 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.

  1. 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.

  1. 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.

Flags: needinfo?(gijskruitbosch+bugs)

Great, then i start to work on it :)

Assignee: nobody → smurfd

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?

Attached patch bug1519365-190527-2050.patch (deleted) — Splinter Review

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...

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Nicklas Boman [:smurfd] from comment #10)

Created attachment 9067866 [details] [diff] [review]
bug1519365-190527-2050.patch

Im 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,

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015

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!

Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9065893 - Attachment description: Bug 1519365 - Update object property names that get passed to loadURIOptions to match the names in loadURIOptions - CPP → Bug 1519365 - Update object property names that get passed to loadURIOptions to match the names in loadURIOptions

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

(In reply to Tim Nguyen :ntim from comment #12)

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

Yes, I'll do that once this lands.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/532685a47a71 Update object property names that get passed to loadURIOptions to match the names in loadURIOptions r=Gijs
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Blocks: 1558145
Blocks: 1558149
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: