Closed Bug 1317990 Opened 3 years ago Closed 3 years ago

Remove Event::IsChrome

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

We should use CallerType instead.
Ben, could you look at the serviceworker bits?
Attachment #8811300 - Flags: review?(bugs)
Attachment #8811300 - Flags: review?(bkelly)
Comment on attachment 8811300 [details] [diff] [review]
Remove Event::IsChrome in favor of passing CallerType arguments

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

r=me with comments addressed.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +772,5 @@
>    mPromises.AppendElement(&aArg);
>  }
>  
>  void
> +FetchEvent::PreventDefault(JSContext* aCx, CallerType aCallerType)

You could actually assert this is not a system caller.  We explicitly don't support system principal service workers.  But I guess probably not necessary.

::: dom/workers/ServiceWorkerEvents.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_workers_serviceworkerevents_h__
>  #define mozilla_dom_workers_serviceworkerevents_h__
>  
> +#include "mozilla/dom/BindingDeclarations.h"

nit: This header should not be needed here.  The signature is defined in the parent class.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1542,5 @@
> +      // one?  The former lies about cases when system code (e.g. default
> +      // handling) does the preventDefault and claims default was _not_
> +      // prevented...  Keep calling the "scriptable" one for now.
> +      CallerType callerType = aWorkerPrivate->UsesSystemPrincipal() ?
> +        CallerType::System : CallerType::NonSystem;

Please just MOZ_ASSERT(!aWorkerPrivate->UsesSystemPrincipal()) and pass CallerType::NonSystem.  We don't allow system principal service workers and have no plans to add them.

I think this means the XXX comment can go as well?
Attachment #8811300 - Flags: review?(bkelly) → review+
Attachment #8811300 - Flags: review?(bugs) → review+
> I think this means the XXX comment can go as well?

Yes.  Applied the other review comments.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/482ed3f75e5b
Remove Event::IsChrome in favor of passing CallerType arguments.  r=smaug,bkelly
https://hg.mozilla.org/mozilla-central/rev/482ed3f75e5b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.