1757808 - Merge Underlying{Sink,Source}*Callback class sets into a single class for each set

Status: RESOLVED FIXED

(Core :: DOM: Streams, task)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Having separate callback classes requires implementing all four classes whenever we get a new native use (currently we have BodyStream* where the start callback is a bit hacky - bug 1746077). And sometimes one of the callbacks is just like "let algorithm be a step that returns undefined", currently doing so requires a full class definition, which is way too much.

Since AFAICT those callbacks are always used together, having a class-as-a-callback-set will shorten the lines and simplify the cycle collection.

(Chrome is seemingly also doing that, so I believe doing the same here shouldn't mean we risk diverting too much from the spec.)

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

I appreciate the current design for being very easy to map from specification to implementation. Having said that, I agree that it leads to lots of boiler plate.

Some questions:

1. You've been working on reducing boilerplate with the macros: Do you see this as alternative approach and we'd skip the macro patches then?
2. You link to Bug 1746077; I agree we'd have a cleaner situation this way. However, this also makes me wonder: we get into situations like var source = { pull(controller) { ... } }, where the Pull callback is IDL provided, but the start and cancel callbacks would be the default implementation. How does that end up looking in this world where we have a single object?

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

1. You've been working on reducing boilerplate with the macros: Do you see this as alternative approach and we'd skip the macro patches then?

Yup, with merged classes I don't think the macros are needed.

2. You link to Bug 1746077; I agree we'd have a cleaner situation this way. However, this also makes me wonder: we get into situations like var source = { pull(controller) { ... } }, where the Pull callback is IDL provided, but the start and cancel callbacks would be the default implementation. How does that end up looking in this world where we have a single object?

Maybe the IDL helper class can include the default implementation in that case?

// pseudocode...
class IDLUnderlyingSourceCallbacks {
  pull() {
    if (mPullCallback) {
      mPullCallback->Call();
      return;
    }
    DefaultImpl();
  }
}

Edit: And if any native class ever need the default algorithms, then we can expose them as static functions.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Ok; I think that this will be a valuable exploration and potentially compelling cleanup.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

BTW, with that same logic the existing IDL helper classes could also do that. https://searchfox.org/mozilla-central/rev/48c71577cbb4ce5a218cf4385aff1ff244dc4432/dom/streams/ReadableByteStreamController.cpp#561-564

Was there reasons not to include the default impls in the IDL helpers?

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

I think it didn't quite occur to me; it is a slight violation of the single responsibility principle, but, it definitely could have been done.. but the inlining of the default algorithm was to avoid having yet-another-object in the default case.

I am less enamoured of the decision than I was when I first made it.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Okay, I'd like to do this before I add any native helpers for transfer/transform things.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1757808 - Part 1: Merge SinkCallback classes as UnderlyingSinkAlgorithms r=mgaudet

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1757808 - Part 2: Merge SourceCallback classes as UnderlyingSourceAlgorithms r=mgaudet

Depends on D140280

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90c80d7dc7a8
Part 1: Merge SinkCallback classes as UnderlyingSinkAlgorithms r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/919e468d0f28
Part 2: Merge SourceCallback classes as UnderlyingSourceAlgorithms r=mgaudet

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/90c80d7dc7a8
https://hg.mozilla.org/mozilla-central/rev/919e468d0f28