Bug 1513241 Comment 19 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

>+++ b/docshell/base/nsDocShell.cpp
>+nsDocShell::LoadURIWithOptions(const nsAString& aURI,

Why not just call this loadURI?  There is no more "non-options" version, and the fact that it has options is obvious from the options member...

>+                               const mozilla::dom::LoadArgs& aLoadArgs) {

This file has "using namespace mozilla::dom", so I don't think we need the full namespacing here.

>+  nsCOMPtr<nsIInputStream> postStream;
>+  if (aLoadArgs.mPostStream.WasPassed()) {
>+    postStream = aLoadArgs.mPostStream.Value();
>+  }

Since we should allow null for the postStream anyway, with null representing "no post data", the IDL should probably just have:

  InputStream? postStream = null;

and then this code can be:

  nsCOMPtr<nsIInputStream> postStream = aLoadArgs.mPostStream;

>+  if (aLoadArgs.mHeaders.WasPassed()) {

Same thing here; I think we should just allow passing null to represent "no header stream" and set that up as the default value.

>+  if (aLoadArgs.mBaseURI.WasPassed()) {

And probably same here; might as well allow passing null explicitly for the base URI.

I don't know whether we'd still want the headerStream and baseURI locals if we do that.

>+  if (aLoadArgs.mReferrerURI.WasPassed()) {

We probably want to allow passing null here too, actually.  Especially since the IDL comments claim something about what happens if it's null.  ;)

>+nsDocShell::LoadURIWithOptionsFromScript(const nsAString& aURI,
>+  dom::LoadArgs loadArgs;

Is the namespace prefixing needed?

>@@ -6905,22 +6917,22 @@ nsresult nsDocShell::EndPageLoad(nsIWebP
>+          dom::LoadArgs loadArgs;

Is the prefixing needed?

>+++ b/docshell/base/nsIDocShell.idl
>+interface nsIContentSecurityPolicy;

Hmm.  Why is this needed?

>+++ b/docshell/base/nsIWebNavigation.idl
>+#include "mozilla/dom/LoadArgsBinding.h"

Why is this needed?  The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

>+   * @param aLoadArgs
>+   *        A JSObject of the form LoadArgs.webidl holding info like e.g. the

"defined in LoadArgs.webidl", right?

>+++ b/dom/ipc/TabChild.cpp
> +  dom::LoadArgs loadArgs;

Do we need the prefixing?

>+      NS_ConvertUTF8toUTF16(aURI),loadArgs);

Missing space.  Did you clang-format this?  If not, please do.

>+++ b/dom/security/FramingChecker.cpp
>@@ -252,18 +253,21 @@ static bool ShouldIgnoreFrameOptions(nsI
>+          dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/dom/webidl/LoadArgs.webidl
>+dictionary LoadArgs {

This name seems very generic...  How about LoadURIOptions (and rename the .webidl too).

>+   * The principal that initiated the load.
>+   */
>+  required Principal? triggeringPrincipal;

There was more documentation in the .idl.  Is leaving it out purposeful?

I'm a bit torn on whether this should be in the dictionary or just an argument, given the "required" status.  Especially given the C++ callers, who can easily forget to set the member...

That said, if we're planning to just pass around the entire LoadURIOptions struct as a whole, it makes sense to have this in the dictionary.

Maybe we should add a constructor to dictionaries with required args that takes those args... Maybe file a followup on that and reference this dictionary?

>+   * One of the REFERRER_POLICY_* constants from nsIHttpChannel.
>+   *  Normal case is REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE.

I know you just copied this comment, but seems to be the "normal case" is UNSET and then the internals will do something with that, right?

>+++ b/editor/composer/nsEditingSession.cpp
>+      dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/xpfe/appshell/nsWebShellWindow.cpp
>+    dom::LoadArgs loadArgs;

Do we need the prefixing?

r=me with those bits fixed.  Thank you for doing this!
```
>+++ b/docshell/base/nsDocShell.cpp
>+nsDocShell::LoadURIWithOptions(const nsAString& aURI,
```
Why not just call this loadURI?  There is no more "non-options" version, and the fact that it has options is obvious from the options member...

```
>+                               const mozilla::dom::LoadArgs& aLoadArgs) {
```
This file has "using namespace mozilla::dom", so I don't think we need the full namespacing here.

```
>+  nsCOMPtr<nsIInputStream> postStream;
>+  if (aLoadArgs.mPostStream.WasPassed()) {
>+    postStream = aLoadArgs.mPostStream.Value();
>+  }
```
Since we should allow null for the postStream anyway, with null representing "no post data", the IDL should probably just have:

```
  InputStream? postStream = null;
```
and then this code can be:

```
  nsCOMPtr<nsIInputStream> postStream = aLoadArgs.mPostStream;
```

```
>+  if (aLoadArgs.mHeaders.WasPassed()) {
```
Same thing here; I think we should just allow passing null to represent "no header stream" and set that up as the default value.

```
>+  if (aLoadArgs.mBaseURI.WasPassed()) {
```
And probably same here; might as well allow passing null explicitly for the base URI.

I don't know whether we'd still want the headerStream and baseURI locals if we do that.

```
>+  if (aLoadArgs.mReferrerURI.WasPassed()) {
```
We probably want to allow passing null here too, actually.  Especially since the IDL comments claim something about what happens if it's null.  ;)

```
>+nsDocShell::LoadURIWithOptionsFromScript(const nsAString& aURI,
>+  dom::LoadArgs loadArgs;
```
Is the namespace prefixing needed?

```
>@@ -6905,22 +6917,22 @@ nsresult nsDocShell::EndPageLoad(nsIWebP
>+          dom::LoadArgs loadArgs;
```
Is the prefixing needed?

```
>+++ b/docshell/base/nsIDocShell.idl
>+interface nsIContentSecurityPolicy;
```
Hmm.  Why is this needed?

```
>+++ b/docshell/base/nsIWebNavigation.idl
>+#include "mozilla/dom/LoadArgsBinding.h"
```
Why is this needed?  The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

```
>+   * @param aLoadArgs
>+   *        A JSObject of the form LoadArgs.webidl holding info like e.g. the
```
"defined in LoadArgs.webidl", right?

```
>+++ b/dom/ipc/TabChild.cpp
> +  dom::LoadArgs loadArgs;
```
Do we need the prefixing?

```
>+      NS_ConvertUTF8toUTF16(aURI),loadArgs);
```
Missing space.  Did you clang-format this?  If not, please do.

```
>+++ b/dom/security/FramingChecker.cpp
>@@ -252,18 +253,21 @@ static bool ShouldIgnoreFrameOptions(nsI
>+          dom::LoadArgs loadArgs;
```
Do we need the prefixing?

```
>+++ b/dom/webidl/LoadArgs.webidl
>+dictionary LoadArgs {
```
This name seems very generic...  How about LoadURIOptions (and rename the .webidl too).

```
>+   * The principal that initiated the load.
>+   */
>+  required Principal? triggeringPrincipal;
```
There was more documentation in the .idl.  Is leaving it out purposeful?

I'm a bit torn on whether this should be in the dictionary or just an argument, given the "required" status.  Especially given the C++ callers, who can easily forget to set the member...

That said, if we're planning to just pass around the entire LoadURIOptions struct as a whole, it makes sense to have this in the dictionary.

Maybe we should add a constructor to dictionaries with required args that takes those args... Maybe file a followup on that and reference this dictionary?

```
>+   * One of the REFERRER_POLICY_* constants from nsIHttpChannel.
>+   *  Normal case is REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE.
```
I know you just copied this comment, but seems to be the "normal case" is UNSET and then the internals will do something with that, right?

```
>+++ b/editor/composer/nsEditingSession.cpp
>+      dom::LoadArgs loadArgs;
```
Do we need the prefixing?

```
>+++ b/xpfe/appshell/nsWebShellWindow.cpp
>+    dom::LoadArgs loadArgs;
```
Do we need the prefixing?

r=me with those bits fixed.  Thank you for doing this!

Back to Bug 1513241 Comment 19