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

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Marion Daly [:mdaly]]

That makes sense to me, we wouldn't want to cause any issues with uplift.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

There merge has happened, so I'm going to do this now.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Exclusive mutex acquisition acknowledged.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make the tree compile again after moving the code to dom/serviceworkers. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8366c96447a2166ca4099fb9f9f3e6fec00876c7

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a290968725f1cd679d77e3d8d141012f49a72a6

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf01a558e58c00d9f487a22d0b0e5d6c4f3eb9e

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

While I'm roto-tilling everything I'm going to remove the workers namespace from serviceworker code as well.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Oh, and I have to fix a ton of paths in tests...

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove workers namespace from service worker code. r=asuth

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove workers namespace from service worker code. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a81e6e1e86de4ea76cab903ad7f910e7f6bde4e3

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Move code, tests, and moz.build rules to dom/serviceworkers. r=asuth

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make the tree compile again after moving the code to dom/serviceworkers. r=asuth

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove workers namespace from service worker code. r=asuth

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.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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".

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove workers namespace from service worker code. r=asuth

See comment 18 and comment 20.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

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

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8945810 [details] [diff] [review]
P4 Fix mochitest paths for new dom/serviceworkers location. r=asuth

See comment 22.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(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 25

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5cbdc4e31b0f
https://hg.mozilla.org/mozilla-central/rev/cd4560fcb554
https://hg.mozilla.org/mozilla-central/rev/8d8c6cf162ed
https://hg.mozilla.org/mozilla-central/rev/c8f0fb23b6e4