Implement support for Service-Worker-Allowed header and associated rules

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: Ehsan)

Tracking

33 Branch
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 1

4 years ago
Created attachment 8602152 [details] [diff] [review]
Part 1: Store the value of the Service-Worker-Allowed header in the CompareManager object
Attachment #8602152 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 2

4 years ago
Created attachment 8602153 [details] [diff] [review]
Part 2: Honor the Service-Worker-Allowed header when prefix matching the service worker scope
Attachment #8602153 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 3

4 years ago
Created attachment 8602154 [details] [diff] [review]
Part 3: Add unit tests for the handling of the Service-Worker-Allowed header
Attachment #8602154 - Flags: review?(nsm.nikhil)
Attachment #8602152 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8602153 [details] [diff] [review]
Part 2: Honor the Service-Worker-Allowed header when prefix matching the service worker scope

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +485,5 @@
>  
>  namespace {
> +enum ScopeStringPrefixMode {
> +  eUseDirectory,
> +  eUsePath

Nit: Could you add a comment that this enum is used to distinguish between a unset SWAllowed header and a set header. The directory/path difference isn't very obvious. Best would be quoting parts of the spec verbatim.

@@ +500,5 @@
>  
> +  if (aPrefixMode == eUseDirectory) {
> +    nsCOMPtr<nsIURL> scriptURL(do_QueryInterface(aScriptURI));
> +    if (NS_WARN_IF(!scriptURL)) {
> +      return rv;

Nit: rv is not affected by the QI. NS_ERROR_FAILURE?
Attachment #8602153 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8602154 [details] [diff] [review]
Part 3: Add unit tests for the handling of the Service-Worker-Allowed header

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

The actual worker files are missing from the patch! Please add them before landing.

::: dom/workers/test/serviceworkers/test_service_worker_allowed.html
@@ +63,5 @@
> +  }
> +
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.serviceWorkers.enabled", true],

also set exemptFromPerDomainMax
Attachment #8602154 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Comment on attachment 8602154 [details] [diff] [review]
> Part 3: Add unit tests for the handling of the Service-Worker-Allowed header
> 
> Review of attachment 8602154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The actual worker files are missing from the patch! Please add them before
> landing.

They're there, but Splinter doesn't display empty files!

FWIW this patch touches code added in bug 1154494, so in order to not make your life more difficult than it already is, I'll wait for that bug to land first.  Please let me know if you don't expect that to happen soon-ish.
Depends on: 1154494
https://hg.mozilla.org/mozilla-central/rev/7409a86dcf41
https://hg.mozilla.org/mozilla-central/rev/78224d2bdde4
https://hg.mozilla.org/mozilla-central/rev/db23832bc281
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.