Closed Bug 1308287 Opened 3 years ago Closed 3 years ago

Make DataTransfer.types use NeedsSubjectPrincipal

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

No description provided.
The idea is to not make consumers think about whether the principal exists or
not when the caller knows for sure that it does.

The substantive changes are in dom/bindings, nsHTMLDocument::SetDesignMode, and
around the CanUseStorage bits.  Everything else is pretty mechanical.
Attachment #8798738 - Flags: review?(amarchesini)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8798738 [details] [diff] [review]
part 1.  Change [NeedsSubjectPrincipal] to only do the Maybe thing for interfaces that can be exposed to workers

Review of attachment 8798738 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +7011,5 @@
> +                JSCompartment* compartment = js::GetContextCompartment(cx);
> +                MOZ_ASSERT(compartment);
> +                JSPrincipals* principals = JS_GetCompartmentPrincipals(compartment);
> +                """)
> +                

extra spaces.
Attachment #8798738 - Flags: review?(amarchesini) → review+
Comment on attachment 8798739 [details] [diff] [review]
part 2.  Make DataTransfer.types use [NeedsSubjectPrincipal]

Review of attachment 8798739 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the biggest fan of having the type of the principal argument be nsIPrincipal& instead of nsIPrincipal*, but I understand that that is probably done to make it clear that null isn't a valid value for the argument.

I'm fine with it, but it does seem like the APIs we are creating with WebIDL aren't the same as the ones we would write manually which feels unfortunate.

::: dom/events/DataTransfer.h
@@ +17,5 @@
>  #include "nsCycleCollectionParticipant.h"
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/EventForwards.h"
> +#include "mozilla/Maybe.h"

I don't think we need to include this header anymore.
Attachment #8798739 - Flags: review?(michael) → review+
> but it does seem like the APIs we are creating with WebIDL aren't the same as the ones we would write manually

I would totally write it with nsIPrincipal& if writing manually if it's not allowed to be null.

> I don't think we need to include this header anymore.

Good catch.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0c2cad052c
part 1.  Change [NeedsSubjectPrincipal] to only do the Maybe thing for interfaces that can be exposed to workers.  r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1078ea4e6d
part 2.  Make DataTransfer.types use [NeedsSubjectPrincipal].  r=mystor
I updated the docs to document the new nsIPrincipal& thing.
https://hg.mozilla.org/mozilla-central/rev/3e0c2cad052c
https://hg.mozilla.org/mozilla-central/rev/0c1078ea4e6d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.