Closed Bug 1430139 Opened 2 years ago Closed 2 years ago

move code from dom/workers to new dom/serviceworkers directory

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 8 obsolete files)

119.26 KB, patch
asuth
: review+
Details | Diff | Splinter Review
13.69 KB, patch
asuth
: review+
Details | Diff | Splinter Review
199.26 KB, patch
asuth
: review+
Details | Diff | Splinter Review
80.50 KB, patch
asuth
: review+
Details | Diff | Splinter Review
With the addition of more IPC actors we're going to start growing a lot more service worker related code.  Lets move what we currently have out of dom/workers and into a new dom/serviceworkers directory.  But we should wait until after the merge on Jan 22 to avoid any uplift headaches before the release.
That makes sense to me, we wouldn't want to cause any issues with uplift.
Priority: -- → P2
There merge has happened, so I'm going to do this now.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Exclusive mutex acquisition acknowledged.
Sorry, I need to pause doing this to fix bug 1433044.  Andrew, if you want to proceed with your work I can postpone this further.
While I'm roto-tilling everything I'm going to remove the workers namespace from serviceworker code as well.
Oh, and I have to fix a ton of paths in tests...
This passes tests locally.  If any new automation failures crop up I'll do follow-on patches or tweak these after review.  Going to flag now.
Andrew, this patch does an hg mv of the service worker code from dom/workers to dom/serviceworkers.  Note, splinter helpfully does not show this at all.  You have to look at the details diff.

The only changes splinter shows here are for the moz.build file changes.  I further tweak the moz.build and other fixups in later patches.
Attachment #8945602 - Attachment is obsolete: true
Attachment #8945678 - Flags: review?(bugmail)
This patch is the result of just fixing compile errors I ran into after doing the move.  Its basically a mix of path changes, unified build fallout, moz.build fixes, and eslint config.
Attachment #8945246 - Attachment is obsolete: true
Attachment #8945679 - Flags: review?(bugmail)
This patch removes any vestige of the mozilla::dom::workers namespace.  Instead serviceworkers now just live in mozilla::dom.  It also installs all exported headers in "mozilla/dom" instead of any of the various insane workers subdirectories.

I also tweaked some of the named runnable names to reflect the new namespace.  I think thats ok, but could put them back if necessary.
Attachment #8945677 - Attachment is obsolete: true
Attachment #8945680 - Flags: review?(bugmail)
Comment on attachment 8945676 [details] [diff] [review]
P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

This patch fixes the many path-specific references in our serviceworker mochitests.  The vast majority of these were simple string replacements, but a few were more tricky.  For example, test_controller.html does a string search for "serviceworkers/controller" that had to be changed to "serviceworkers/test/controller".
Attachment #8945676 - Flags: review?(bugmail)
P3 needs a small fix to re-declare ServiceWorkerVisible() in ServiceWorkerEvents.h to satisfy FetchEvent.webidl usage.  Sadly webidl codegen doesn't let us easily provide a header for a [Func=].

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a67a3379431363d9ad5c1d172a89188313dad379
See comment 18 and comment 20.
Attachment #8945680 - Attachment is obsolete: true
Attachment #8945680 - Flags: review?(bugmail)
Attachment #8945808 - Flags: review?(bugmail)
See comment 19.

Also, I updated test_cookie_fetch.html to do an includes on the cookie string instead of an exact match.  It seems on android the test is now run in with another test that leaves a cookie on the test domain.  I figured it was better to make the cookie test resilient vs trying to hunt down leaked cookies everywhere.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc525359169533ebc5725e841b2ba6ba10ebaa6a
Attachment #8945676 - Attachment is obsolete: true
Attachment #8945676 - Flags: review?(bugmail)
Comment on attachment 8945810 [details] [diff] [review]
P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

See comment 22.
Attachment #8945810 - Flags: review?(bugmail)
(In reply to Ben Kelly [:bkelly] from comment #20)
> P3 needs a small fix to re-declare ServiceWorkerVisible() in
> ServiceWorkerEvents.h to satisfy FetchEvent.webidl usage.  Sadly webidl
> codegen doesn't let us easily provide a header for a [Func=].

I understand this to be similar to the discussion in bug 1428927, particularly as described at https://bugzilla.mozilla.org/show_bug.cgi?id=1428927#c5.  Specifically:
- FetchEvent defines a custom headerFile of ServiceWorkerEvents.h in Bindings.conf.
- The use of a custom header file causes the binding magic at https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/dom/bindings/Codegen.py#1206 to not try and automatically generate an include.
- The binding magic is dependent on seeing something that looks like "mozilla::dom::ClassWithAHeader::Func" because it will generate `#include "mozilla/dom/ClassWithAHeader.h"` iff it sees a "::".  (The pre-patch value of "mozilla::dom::workers::ServiceWorkerVisible" in contrast would have resulted in an include of "mozilla/dom/workers.h" if not inhibited by heuristic.)

So the change to ServiceWorkerEvents.h seems appropriate, and in the future I guess we could maybe move to FetchEvent.h and away from the custom header file, but ServiceWorkerVisible also needs to move onto a class.
Comment on attachment 8945810 [details] [diff] [review]
P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

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

Re: comment 19, Yeah, a lot of these path transforms are tongue twistery in nature!

::: dom/serviceworkers/test/test_cookie_fetch.html
@@ +35,4 @@
>        } else if (e.data.status == "done") {
> +        // Note, we can't do an exact is() comparison here since other
> +        // tests can leave cookies on the domain.
> +        ok(e.data.cookie.includes("foo=bar"),

Affirming non-path-related change explained in comment 22.
Attachment #8945810 - Flags: review?(bugmail) → review+
Attachment #8945679 - Flags: review?(bugmail) → review+
Comment on attachment 8945678 [details] [diff] [review]
P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth

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

restating the moves:
- dom/workers/*Service*.* => dom/serviceworkers/*
- dom/workers/test/serviceworkers/** => dom/serviceworkers/test/*

::: dom/workers/moz.build
@@ -36,5 @@
>  ]
>  
>  # Stuff needed for the bindings, not really public though.
>  EXPORTS.mozilla.dom.workers.bindings += [
> -    'ServiceWorker.h',

Restating: We were installing/exporting dom/workers/ServiceWorker.h to mozilla/dom/workers/bindings, and we were doing that because Bindings.conf has been configured to explicitly use a headerFile at that path since the initial binding landing in bug 930348.  This, at least based on current bindings code, seems to have been an decision to attempt to imitate the dom/binding source tree and auto-generated binding tree hierarchy with the namespace rendered explicit or something?  Or maybe the binding generator really needed this help back then.  Either way, it just made things more confusing in modern times, so it's nice this is going away.
Attachment #8945678 - Flags: review?(bugmail) → review+
Comment on attachment 8945808 [details] [diff] [review]
P3 Remove workers namespace from service worker code. r=asuth

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

Thank you for this fantastic cleanup of the dom/workers/ tree.  While this is "merely" a syntactic cleanup that pales in the face of the amazing semantic cleanups you have been doing, it's clearly not a small undertaking and is also a major step forward in making this massive codebase seem less overwhelming.

::: dom/bindings/Bindings.conf
@@ -323,5 @@
>  },
>  
>  'ExtendableEvent': {
>      'headerFile': 'mozilla/dom/ServiceWorkerEvents.h',
> -    'nativeType': 'mozilla::dom::workers::ExtendableEvent',

Affirming removal of custom nativeType mapping for the bindings in this file that have moved from the mozilla::dom::workers namespace into the mozilla::dom namespace.

@@ -763,5 @@
>      'nativeType': 'nsScreen',
>  },
>  
> -'ServiceWorker': {
> -    'nativeType': 'mozilla::dom::workers::ServiceWorker',

Same rationale as above plus my comments in the part 1 patch.  We don't need this weird hack anymore.

::: dom/webidl/FetchEvent.webidl
@@ +7,5 @@
>   * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
>   */
>  
>  [Constructor(DOMString type, FetchEventInit eventInitDict),
> + Func="ServiceWorkerVisible",

Affirming: Unified-safe because a declaration has been added to ServiceWorkerEvents.h for ServiceWorkerVisible, and ServiceWorkerEvents is the `headerFile` for this binding in Bindings.conf.

::: dom/webidl/ServiceWorker.webidl
@@ +9,5 @@
>   */
>  
>  // Still unclear what should be subclassed.
>  // https://github.com/slightlyoff/ServiceWorker/issues/189
> +[Func="ServiceWorkerVisible",

Affirming: Unified-safe because a declaration exists in ServiceWorker.h for ServiceWorkerVisible, and ServiceWorker.h is the default binding header for this binding now that the custom entry in Bindings.conf has been removed.
Attachment #8945808 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbdc4e31b0f
P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4560fcb554
P2 Make the tree compile again after moving the code to dom/serviceworkers. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8c6cf162ed
P3 Remove workers namespace from service worker code. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f0fb23b6e4
P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth
You need to log in before you can comment on or make changes to this bug.