1432963 - fix dom/workers exported headers to not be insane

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Our dom/worker headers are currently a bit crazy.  Lots of code needs to include WorkerPrivate.h.  But WorkerPrivate.h uses a bunch of headers that are not referenced by their exported location:

  Workers.h
  WorkerHolder.h
  Queue.h

We could fix these to be:

  mozilla/dom/workers/Workers.h
  mozilla/dom/workers/bindings/WorkerHolder.h

But Queue.h is not exported.  Also, just to make things more confusing, WorkerPrivate.h is exported as:

  mozilla/dom/WorkerPrivate.h

This is just kind of crazy and unworkable.  So we have code all over the place that just adds dom/workers to their moz.build LOCAL_INCLUDES to work around it.  That just leads to the problem getting worse.

We should at least make it possible to include WorkerPrivate.h from the export.

Andrea, any interest in working on this?

Comment 1

[Andrea Marchesini [:baku]]

Yeah, this was part of the Worker cleanup task. I'll take this. I'm keeping the NI flag.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

It would also be nice to just use mozilla::dom:: namespace and not the extra workers:: namespace.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - no workers namespace for SharedWorker

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - Get rid of WorkerCrossThreadDispatcher and WorkerTask

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - WorkerError and WorkerPrincipal without workers namespace

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - WorkerThread without workers namespace

Note that here I had to introduce a workers::WorkerPrivate. This is going to be removed in the following patches.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 5 - WorkerEventTarget in a separate file

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 6 - Get rid of WorkerInlines

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 7 - WorkerDebugger without worker namespace

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 8 - WorkerTargetHolder without worker namespace

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 9 - WorkerLoadInfo

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part A - ScriptLoader into a workerinternals namespace

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part B - MessageEventRunnable without namespace

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part D - WorkerHolder without namespace

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part E - WorkerPrivate without namespace

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part C - WorkerRunnable without namespace

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part F - static function in a workers namespace

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part G - JSSettings in a workerinternals namespace

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part H - no LIBS=[workers] in moz.build files

Comment 20

[Andrea Marchesini [:baku]]

After these patches, here what we have:

In mozilla/dom/ we have these headers:

    'SharedWorker.h',
    'WorkerCommon.h',
    'WorkerDebugger.h',
    'WorkerDebuggerManager.h',
    'WorkerHolder.h',
    'WorkerHolderToken.h',
    'WorkerLoadInfo.h',
    'WorkerLocation.h',
    'WorkerNavigator.h',
    'WorkerPrivate.h',
    'WorkerRunnable.h',
    'WorkerScope.h',

with 'mozilla::dom::' namespace.

there are 3 private exposed headers in mozilla/dom/workerinternal/

    'JSSettings.h',
    'Queue.h',
    'RuntimeService.h',

in 'mozilla::dom::workerinternals::' namespace

WorkerCommon exposes several static functions. Those are into a 'workers' namespace. Those functions are AssertIsOnMainThread() and so on.

No LIBS+=[ '/dom/workers' ] is required anymore.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946741 [details] [diff] [review]
part 1 - no workers namespace for SharedWorker

Thank you. All the worker namespace stuff has been always really weird.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946743 [details] [diff] [review]
part 2 - Get rid of  WorkerCrossThreadDispatcher and WorkerTask

unused code. lovely.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946748 [details] [diff] [review]
part 5 - WorkerEventTarget in a separate file

'WorkerLoadInfo.h' changes look unrelated. Which patch removes BEGIN_WORKERS_NAMESPACE from that file?
So far I have found it. Perhaps some latter patch. I may change r- to r+ if I find it.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946752 [details] [diff] [review]
part 9 - WorkerLoadInfo

Ahaa, here.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946755 [details] [diff] [review]
part D - WorkerHolder without namespace

ok, this changes also NotificationEvent

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946762 [details] [diff] [review]
part C - WorkerRunnable without namespace

I should have looked at these in order, but oh well.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946764 [details] [diff] [review]
part F - static function in a workers namespace

Personally I really don't like functions in some namespace. Having a class and static methods just makes it easier to figure out where the implementation is etc.

But looks like this is just following the existing pattern in workers, so fine.

What is the change to layout/generic/WritingModes.h? Pleaes explain that, and r+

Comment 28

[Andrea Marchesini [:baku]]

> What is the change to layout/generic/WritingModes.h? Pleaes explain that,
> and r+

There is a conflict with 2 different Side implementation. I had to force the full namespace in order to avoid a conflict.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946765 [details] [diff] [review]
part G - JSSettings in a workerinternals namespace

Moving CancelWorkersForWindow etc around in the file is a tad annoying. But perhaps the context in the patch just didn't quite reveal the reason.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8946766 [details] [diff] [review]
part H - no LIBS=[workers] in moz.build files

Make sure to push all the patches to try (building on all the platforms) before pushing to m-i ;)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e5e7c4cf88
Fixing workers headers - part 1 - no workers namespace for SharedWorker, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c06802f7ce4
Fixing workers headers - part 2 - Get rid of WorkerCrossThreadDispatcher and WorkerTask, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c3ec4cbe78
Fixing workers headers - part 3 - WorkerError and WorkerPrincipal without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f2bea31ba9
Fixing workers headers - part 4 - WorkerThread without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/89714cb6e883
Fixing workers headers - part 5 - WorkerEventTarget in a separate file, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8d6c014e67
Fixing workers headers - part 6 - Get rid of WorkerInlines.h, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6978eec68f5f
Fixing workers headers - part 7 - WorkerDebugger without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a76ef9f9fd5
Fixing workers headers - part 8 - WorkerTargetHolder without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83b357dc475
Fixing workers headers - part 9 - WorkerLoadInfo without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5abdabc30f
Fixing workers headers - part 10 - ScriptLoader into a workerinternals namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e7ac848a5d
Fixing workers headers - part 11 - MessageEventRunnable without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/765fff03aebd
Fixing workers headers - part 12 - WorkerRunnable without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/68a1d607cbc4
Fixing workers headers - part 13 - WorkerHolder without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ab83c80c5e
Fixing workers headers - part 14 - WorkerPrivate without workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a858acf697e
Fixing workers headers - part 15 - static function in a workers namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f82c8af2c1ea
Fixing workers headers - part 16 - JSSettings in a workerinternals namespace, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/001c306fa281
Fixing workers headers - part 17 - no LIBS=[workers] in moz.build files, r=smaug

Comment 32

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d7e5e7c4cf88
https://hg.mozilla.org/mozilla-central/rev/5c06802f7ce4
https://hg.mozilla.org/mozilla-central/rev/d3c3ec4cbe78
https://hg.mozilla.org/mozilla-central/rev/a8f2bea31ba9
https://hg.mozilla.org/mozilla-central/rev/89714cb6e883
https://hg.mozilla.org/mozilla-central/rev/2d8d6c014e67
https://hg.mozilla.org/mozilla-central/rev/6978eec68f5f
https://hg.mozilla.org/mozilla-central/rev/7a76ef9f9fd5
https://hg.mozilla.org/mozilla-central/rev/b83b357dc475
https://hg.mozilla.org/mozilla-central/rev/0f5abdabc30f
https://hg.mozilla.org/mozilla-central/rev/69e7ac848a5d
https://hg.mozilla.org/mozilla-central/rev/765fff03aebd
https://hg.mozilla.org/mozilla-central/rev/68a1d607cbc4
https://hg.mozilla.org/mozilla-central/rev/22ab83c80c5e
https://hg.mozilla.org/mozilla-central/rev/7a858acf697e
https://hg.mozilla.org/mozilla-central/rev/f82c8af2c1ea
https://hg.mozilla.org/mozilla-central/rev/001c306fa281

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

FYI, it looks like there are some lingering:

using namespace mozilla::dom::workers;
using namespace workers;

in the tree.  Also maybe some forward declarations like this:

https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/dom/canvas/ImageBitmap.h#42

Comment 34

[Andrea Marchesini [:baku]]

Filing a separate bug for that.