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

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P5)

Description

[Christoph Kerschbaumer [:ckerschb}]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

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

Comment 2

[Marco Bonardo [:mak]]

Gijs, are you interested into mentoring this bug?

Comment 3

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1517703 looks like it'll generate more work to do here.

Comment 5

[Nicklas Boman [:smurfd]]

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

Comment 6

[:Gijs (he/him)]

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

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 ?

Yes, in relation to loadURIOptions and nsDocShell.cpp/h and friends.

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

Comment 7

[Nicklas Boman [:smurfd]]

Great, then i start to work on it :)

Comment 8

[Nicklas Boman [:smurfd]]

Attachment: Bug 1519365 - Update object property names that get passed to loadURIOptions to match the names in loadURIOptions

Comment 9

[Nicklas Boman [: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?

Comment 10

[Nicklas Boman [:smurfd]]

Attachment: 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,
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

[:Gijs (he/him)]

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

Comment 12

[Tim Nguyen :ntim]

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

[:Gijs (he/him)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/532685a47a71