Closed Bug 1130101 Opened 9 years ago Closed 9 years ago

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

Categories

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

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: nsm, Assigned: ehsan.akhgari)

References

Details

Attachments

(3 files)

Assignee: nobody → ehsan
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+
(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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.