1269162 - Remove separate worker binding for XMLHttpRequest and friends

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This likely requires introducing an abstract base class that can be shared between the main thread and worker thread implementations.

Comment 1

[Andrea Marchesini [:baku]]

This is the last pure worker binding. All the others have been removed or they have a patch ready to be reviewed.
I think it's time to think about how/when rewrite XHR from scratch.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - XMLHttpRequestWorker

I did this task in 6 patches. The first patch is about renaming dom/workers/XMLHttpRequest.* to XMLHttpRequestWorker. This will be useful in the next patches.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - XMLHttpRequestUploadWorker

Also this second patch is just about renaming things.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - dom/xhr

Third step is moving everything in dom/xhr

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 4 - XMLHttpRequestEventTarget

Separate files for XMLHttpRequestEventTarget

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 5 - XMLHttpRequestUpload

Unify XMLHttpRequestUpload in 1 single implementation.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 6 - XMLHttpRequest

This last step puts everything together. We have 3 classes in total for XMLHttpRequest:

1. XMLHttpRequest - WebIDL class with virtual methods.
2. XMLHttpRequestWorker - Worker implementation
3. XMLHttpRequestMainThread - Main-Thread implementation

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 6 - XMLHttpRequest

Comment 9

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

Comment on attachment 8763700 [details] [diff] [review]
part 1 - XMLHttpRequestWorker

> 'XMLHttpRequest': [
> {
>     'nativeType': 'nsXMLHttpRequest',
>     'implicitJSContext': [ 'send'],
> },
> {
>+    'nativeType': 'mozilla::dom::workers::XMLHttpRequestWorker',
>+    'headerFile': 'mozilla/dom/workers/bindings/XMLHttpRequestWorker.h',
I'm pretty sure the headerFile is wrong, pointing to some non-existing file.
Does this actually compile? But since the rest of the patch is just renaming, r+, but fix this.

Comment 10

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

Comment on attachment 8763705 [details] [diff] [review]
part 5 - XMLHttpRequestUpload


>+private:
>+  virtual ~XMLHttpRequestUpload()
>+  {}
>+
>+  RefPtr<workers::XMLHttpRequestWorker> mXHR;
Why do we need this? I see it is copied from worker implementation, but rather annoying to add to this generic implementation.
Please add a comment.

Comment 11

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

Comment on attachment 8763702 [details] [diff] [review]
part 3 - dom/xhr

This is of course totally unrelated patch, so could (or should) have been done in a separate bug.
But I can see doing this here helps with testing.

I tried to check that we aren't losing any tests. Hopefully I didn't miss anything.

Comment 12

[Andrea Marchesini [:baku]]

> >+    'nativeType': 'mozilla::dom::workers::XMLHttpRequestWorker',
> >+    'headerFile': 'mozilla/dom/workers/bindings/XMLHttpRequestWorker.h',

This is correct. in dom/workers/moz.build we have:

EXPORTS.mozilla.dom.workers.bindings += [
 ... XMLHttpRequestWorker.h

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 6 - XMLHttpRequest

The previous version was breaking a test. The issue was about XMLHttpRequestMainThread::SetMozBackgroundRequest: for WebIDL (main-thread) it should not throw errors, for Worker and XPIDL it should.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e12af97993d
part 1 - Move XHR in workers to XMLHttpRequestWorker, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01653e7ad7c
part 2 - Move XHRUpload in workers to XMLHttpRequestUploadWorker, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a63a62c0dd
part 3 - Move XHR code into dom/xhr, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03db893b8da
part 4 - Move XMLHttpRequestEventTarget in separate files, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb3b5ac04af
part 5 - Merge XMLHttpRequestUploadWorker and XMLHttpRequestUpload, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/155012d3d373
part 6 - Merge XMLHttpRequestWorker and XMLHttpRequestMainThread, r=smaug

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7876627840cb
part 7 - A merge error fixed, r=me

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e12af97993d
https://hg.mozilla.org/mozilla-central/rev/c01653e7ad7c
https://hg.mozilla.org/mozilla-central/rev/55a63a62c0dd
https://hg.mozilla.org/mozilla-central/rev/e03db893b8da
https://hg.mozilla.org/mozilla-central/rev/ddb3b5ac04af
https://hg.mozilla.org/mozilla-central/rev/155012d3d373
https://hg.mozilla.org/mozilla-central/rev/7876627840cb