Refactoring BaseAudioContext causes breakages in prototype.

[hoch]

Now BaseAudioContext has the majority of factory methods, so querying hasOwnProperty will not work on the instance of AudioContext of OfflineAudioContext.

AudioContext.prototype.hasOwnProperty(`createGain`); // this returns false.

It turns out there are few libraries doing this to sniff by the factory method name in Web Audio API. (related crbug entry)

@joeberkovitz @mdjp @padenot WDYT?

[padenot]

Do we really have to expose this to the Web ? Is there a mechanism that we could use to share some code in the spec, but we can still implement it as two separate classes ?

[rtoy]

Perhaps #308 (comment) is relevant. Now I'm not sure what @heycam is proposing by "mirror".

[hoch]

@padenot Spec-wise, We are exactly doing what was suggested in #308 by @heycam. I think that was WG's decision? Unfortunately I believe we should either 1) go by the IDL or 2) have two separate AC classes by reverting the spec change.

[hoch]

From developers' perspective, this can be done to check the prototype of BaseAudioContext. BaseAudioContext is not constructible, but still has an accessible prototype interface.

// This returns true with the new IDL.
BaseAudioContext.prototype.hasOwnProperty(`createGain`);

The other alternative is using in.

// This is true as well.
'createGain' in AudioContext.prototype

[hoch]

@padenot @joeberkovitz @rtoy @mdjp

What's the WG's decision on this? If we don't have the decision soon, we might have to revert this change in Chromium.

I am also asking feedback from @domenic from the spec/IDL perspective.

[rtoy]

I don't think what Chrome has done or might do should have any bearing on this decision. We just need to decide if breaking existing uses of hasOwnProperty is intended. Or come up with some other solution while still preserving BaseAudioContext in a meaningful way. Or remove it altogether.

[domenic]

I think the BaseAudioContext factoring is preferable, but if it's not web compatible then I guess there's nothing you can do there and it needs to be reverted.

If you want to share "spec code" between two different classes without using a common base class, you can use the Web IDL "mixin" feature like so:

interface A { }
interface B { }

[NoInterfaceObject]
interface Mixin { ... };

A implements Mixin;
B implements Mixin;

From JS, this is indistinguishable from just inlining the contents of Mixin into both A and B.

You then have to be careful when writing the algorithms for operations on Mixin to make sure they apply in a reasonable way to any target they are mixed in to (i.e. to both A and B).

[hoch]

@domenic Also do we have to consider the size of breakage? Or should we avoid anything breaks the current web? I am just trying to gauge the principle for this type of issue.

[domenic]

In general you want to weigh the size of the breakage vs. the benefit of the breakage. I am not clear on either of those in this context. As Rick said in the crbug, you'll probably want to do some kind of compat analysis. If there are popular libraries which use this pattern then the size of the breakage is likely to be high.

[RByers]

Yes, deciding if something is "web compatible" is a bit of an art form subject to interpretation. We can make a call for blink, but we're sometimes more aggressive about breakage than other vendors are. Are there other implementors impacted by this who want to weigh in on the question of how much breakage is acceptable in this case?

[RByers]

Seems like maybe we have precedent for using the mixin style for this sort of issue. Eg. WebGLRenderingContextBase is [NoInterfaceObject] with WebGLRenderingContext implementing it.

[rtoy]

This seems like a good solution.

[joeberkovitz]

+1 on the interface mixin solution, which seems kind of perfect.

[hoch]

For the sake of the discussion, here is a more complete revision of the IDL.

BaseAudioContext

[NoInterfaceObject]
interface BaseAudioContext {
    readonly        attribute AudioDestinationNode destination;
    readonly        attribute float                sampleRate;
    readonly        attribute double               currentTime;
    readonly        attribute AudioListener        listener;
    readonly        attribute AudioContextState    state;
    Promise<void>          resume ();
    AudioBuffer            createBuffer (...);
    Promise<AudioBuffer>   decodeAudioData (...);
    AudioBufferSourceNode  createBufferSource ();
    ConstantSourceNode     createConstantSource ();
    ScriptProcessorNode    createScriptProcessor (...);
    AnalyserNode           createAnalyser ();
    GainNode               createGain ();
    DelayNode              createDelay (...);
    BiquadFilterNode       createBiquadFilter ();
    IIRFilterNode          createIIRFilter (...);
    WaveShaperNode         createWaveShaper ();
    PannerNode             createPanner ();
    StereoPannerNode       createStereoPanner ();
    ConvolverNode          createConvolver ();
    ChannelSplitterNode    createChannelSplitter (...);
    ChannelMergerNode      createChannelMerger (...);
    DynamicsCompressorNode createDynamicsCompressor ();
    OscillatorNode         createOscillator ();
    PeriodicWave           createPeriodicWave (...);
};

AudioContext

[Constructor]
interface AudioContext : EventTarget {
    readonly        attribute double         baseLatency;
    readonly        attribute double         outputLatency;
                    attribute EventHandler   onstatechange;
    AudioTimestamp                  getOutputTimestamp ();
    Promise<void>                   close ();
    Promise<void>                   suspend ();
    MediaElementAudioSourceNode     createMediaElementSource (...);
    MediaStreamAudioSourceNode      createMediaStreamSource (...);
    MediaStreamAudioDestinationNode createMediaStreamDestination ();
}

OfflineAudioContext

[Constructor(...)]
interface OfflineAudioContext : EventTarget {
    readonly        attribute unsigned long length;
                    attribute EventHandler  oncomplete;
                    attribute EventHandler  onstatechange;
    Promise<AudioBuffer> startRendering ();
    Promise<void>        suspend (...);
}

Interface Composition

AudioContext implements BaseAudioContext;
OfflineAudioContext implements BaseAudioContext;
typedef (AudioContext or OfflineAudioContext) AnyAudioContext;

[foolip]

BaseAudioContext shouldn't inherit from EventTarget, instead both AudioContext and OfflineAudioContext should:

[NoInterfaceObject]
interface BaseAudioContext { ... };

[Constructor]
interface AudioContext : EventTarget { ... };
AudioContext implements BaseAudioContext;

[Constructor(...)]
interface OfflineAudioContext : EventTarget { ... };
OfflineAudioContext implements BaseAudioContext;

[foolip]

I have done a a search for "AudioContext.prototype" in httparchive:har.2016_09_15_chrome_requests_bodies and there were actually only 31 matches:
https://gist.github.com/foolip/93496c2c78ebf552d50849ffd27a4664

Of those, 11 have the string "AudioContext.prototype.hasOwnProperty", and it looks like the source is this:
https://github.com/processing/p5.js/blob/5c5cfa1eb39f29667b88ac054ba3c5ce87236d4d/lib/addons/p5.sound.js#L93

That's not the same as in https://crbug.com/555608 though, so it's not all there is.

Unless there's a tangible advantage to web developers or implementers to have BaseAudioContext on the prototype chain, avoiding the breakage with the mixin solution seems better.

[padenot]

Agreed, a mixin sounds better. Paul.

[domenic]

@domenic - WDYT?

Looks good, apart from the already mentioned issue that the inheritance needs to not happen on the mixin.

[hoch]

One more question: then onstatechange in BaseAudioContext should be moved to each actual context objet? BaseAudioContext does not extend EventTarget any more. Or does this mixin make it automatically work when BAC merges into AC/OAC?

[domenic]

I'm not sure, but I think it should work fine...

[rtoy]

@hoch mentioned to me in passing yesterday that if you do

[NoInterfaceObject]
interface BaseAudioContext { ... }

interface AudioContext : BaseAudioContext { ... }

that AudioContext.prototype.hasOwnProperty would inherit the properties from BaseAudioContext. Is this right? I can't quite figure this out from the WebIDL spec.

[domenic]

No, that's not correct. (Where by "inherit the properties from" I assume you mean "would return true for the properties of".)

That would create something exactly like the current spec except that there would be no window.BaseAudioContext.

[hoch]

No. I was also told it might be the case, but I realized that can't be true because WebIDL says:

However, an instance of Foo can expose the interface prototype object by gettings its internal [[Prototype]] property value – Object.getPrototypeOf(window.foo) in this example.

So it still shows up in the object's prototype, even if it is defined as 'no interface object'. Thus inherited properties will not be 'own' properties.

The point is, Mixin is the only way to resolve this. Since @joeberkovitz @rtoy and @padenot agreed on this, I'll submit a PR to fix this.

[hoch]

PR: #1029

[hoch]

FYI, BaseAudioContext having onstatechange without EventTarget is not possible. I think we need to duplicate onstatechange to each context object. Ugly, but that's life.

[hoch]

I just found a significant issue in the new mixin pattern. This is our new AudioNode constructor's IDL.

[Constructor(BaseAudioContext context, optional GainOptions options)]
interface GainNode : AudioNode {
    readonly        attribute AudioParam gain;
};

Because BaseAudioContext is not exposed, this IDL is not valid anymore. This means that we have to keep a pair of constructors for AudioContext and OfflineAudioContext on every AudioNode. I don't think this worth pursuing at all.

At this point, @rtoy and I believe we should just keep the original design of BaseAudioContext even if it breaks few web pages that are using AudioContext.prototype.hasOwnProperty(). It is unfortunate but the advantages of BaseAudioContext can compensate those losses IMHO.

WDYT?

AudioWG: @joeberkovitz @padenot @mdjp
Spec: @domenic @foolip

[domenic]

Just do typedef AnyAudioContext (AudioContext or OfflineAudioContext) and use that everywhere that should accept either.

[foolip]

FYI, BaseAudioContext having onstatechange without EventTarget is not possible.

Does webidl2.js complain, or is it actually invalid per WebIDL?

[hoch]

Actually, Chromium's IDL parser (or binding) is complaining. I can't compile without EventTarget attached to BaseAudioContext. The warning says there is no valid event listener in BaseAudioContext.

[foolip]

Hmm, probably there's just no support for it, @bashi?

[joeberkovitz]

Here is a summary of the state of play as of yesterday's WG telcon (7 Oct 2016). There were two evenly split schools of thought on the call, which left us with a desire to seek guidance from the WG, and most particularly from implementors of the spec.

A few facts up front, though:

• The change to subclassing BaseAudioContext into AudioContext and OfflineAudioContext produced two different possibilities for breaking current applications:
  • Code that used prototype.hasOwnProperty("someProperty") to sniff AudioContext features can now break because someProperty will now belong to the superclass BaseAudioContext, hence hasOwnProperty() will now return false instead of true. There is a small set of web audio apps or libraries out there that do this.
  • Code that used to assume that OfflineAudioContext was a subclass of AudioContext could break (since some online methods are no longer exposed in OfflineAudioContext where they make no sense). The extent of this usage is unknown and thought to likely be minor.
• Making BaseAudioContext into a mixin resolves the hasOwnProperty() breakage, since the mixin places its elements directly on the surface of each class that uses it. The cost is a more complex approach in the IDL and the spec. Since it is not possible to use a mixin as a concrete type in IDL, a typedef is required (see above). Note that this approach still leaves the potential for\ the OfflineAudioContext inheritance breakage.

The alternative viewpoints can be summarized as:

1. Keep BaseAudioContext as a superclass of the other two context types since this approach is already agreed and feels intuitive to developers. Break the apps, since there's not much usage there, get in touch with the developers to let them know, and. Using prototype.hasOwnProperty("functionName") as opposed to typeof(prototype.functionName) == "function" was already an inherently fragile approach.
2. Move BaseAudioContext to a mixin as described in Use Mixin pattern for contexts #1029, in order to avoid this breakage, and let the spec be a little more baroque.

[RByers]

The primary thing we rely on to resolve such tradeoffs in blink is hard compat data. Usually this means UseCounter data from the wild, although sometimes HTTP Archive data is good enough (or required in addition). So, for example, it should be possible to add a UseCounter that counts usage of the AudioContext members on instances of OfflineAudioContext to precisely quantify the risk of the 2nd breakage you describe (but we'd probably have to revert the refactoring from blink and return to the original design while we collected such data).

If the WG decides to prefer the breaking change, then we'll need to have an "intent to remove" discussion on blink-dev. The typical outcome is to require the above sort of concrete data and a delay of a milestone or two where Chrome generates a deprecation console warning on usage before the breaking change actually takes effect.

[RByers]

Actually, Chromium's IDL parser (or binding) is complaining. I can't compile without EventTarget attached to BaseAudioContext. The warning says there is no valid event listener in BaseAudioContext.

Be careful not to conflate WebIDL requirements with blink bindings generator limitations here. It's not uncommon for IDL in blink to be structured slightly differently from the spec (with equivalent observable behavior) in order to temporarily work around a blink bindings generator issue. Those issues shouldn't influence the spec discussion here - just an implementation detail for us to worry about in blink.

[hoch]

In the long run, exposing BaseAudioContext prototype will be beneficial for developers in terms of the extensibility. (e.g. subclassing, composition) To subclass the gain node factory method, this is what we had to do originally:

var gainFactory = AudioContext.prototype.createGain;
AudioContext.prototype.createSpecialGain = function () { 
  /* do something special */
  var gainNode = gainFactory.call(this);
  gainNode.special = true;
  return gainNode; 
};

Because OfflineAudioContrext extends AudioContext, the following is possible and I find this quite useful especially you want to wrap the factory methods.

var context = new OfflineAudioContext(...);
var specialGain = context.createSpecialGain();

The BaseAudioContext makes this process a lot easier. Adding something to its prototype, then it will be available on any context instance.

BaseAudioContext.prototype.createSpecialFunction= function () { /* construction */ };

Notably, the Mixin pattern will break this example because now AC and OAC are completely different classes. Not sure typedef (AudioContext or OfflineAudioContext) AnyContext can help this problem though.

Also I want to point out that this discussion is not about breaking or not breaking - it is more of "breaking 815 pages (exposing BaseAudioContext)" or "breaking 23 pages (using Mixin)". We have the last resort as well; not changing anything and keep AudioContext-OfflineAudioContext inheritance intact.

I understand the backward compatibility is important but also the proper extensibility is what our spec has been missing for a while. I believe exposing BaseAudioContext and node constructor is the first critical step.

[RByers]

That's a good argument for why it's worth taking some compat pain for this :-). It looks like subclass-ability was part of the TAG feedback too.

@foolip said:

Unless there's a tangible advantage to web developers or implementers to have BaseAudioContext on the prototype chain, avoiding the breakage with the mixin solution seems better.

@foolip, is this enough "tangible advantage" for you? If the WG agrees with @hoch's position, then I guess we should take this to a blink "intent to remove" thread? The main question will be whether we should be giving / can give developers a deprecation warning for at least one milestone before changing both of these things.

I guess we also need some concrete compat data on this:

Code that used to assume that OfflineAudioContext was a subclass of AudioContext could break (since some online methods are no longer exposed in OfflineAudioContext where they make no sense). The extent of this usage is unknown and thought to likely be minor.

That at least is an easy one to add a deprecation warning for for a milestone (which will also give us UseCounter data), right?

[rtoy]

On Fri, Oct 7, 2016 at 1:30 PM, Rick Byers [email protected] wrote:

That's a good argument for why it's worth taking some compat pain for this
:-). It looks like subclass-ability was part of the TAG feedback
#250 too.

@foolip https://github.com/foolip said:

Unless there's a tangible advantage to web developers or implementers to
have BaseAudioContext on the prototype chain, avoiding the breakage with
the mixin solution seems better.

@foolip https://github.com/foolip, is this enough "tangible advantage"
for you? If the WG agrees with @hoch https://github.com/hoch's
position, then I guess we should take this to a blink "intent to remove"
thread? The main question will be whether we should be giving / can give
developers a deprecation warning for at least one milestone before changing
both of these things.

I guess we also need some concrete compat data on this:

Code that used to assume that OfflineAudioContext was a subclass of
AudioContext could break (since some online methods are no longer exposed
in OfflineAudioContext where they make no sense). The extent of this usage
is unknown and thought to likely be minor.

That at least is an easy one to add a deprecation warning for for a
milestone (which will also give us UseCounter data), right?

​I'm a little confused on exactly where and when the deprecation message is
triggered. When someone does AudioContext.prototype.hasOwnProperty?
Something else? (If the former, I'll need help on that.)​

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#1022 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAofPA8yRovmMnsIAAEAHFwwZghiDjFPks5qxqvRgaJpZM4KMD7K
.

Ray

[hoch]

I think the broken AudioContext.prototype.hasOwnProerty() is just one of symptoms from the change in its prototype chain. There might be more breakages.

We can throw a general warning message like "The prototype of AudioContext has been changed. Please refer some.url for the more information" when the context is created.

We also might want to consider writing up a short doc that explains the breakage and the resolution in the wiki here. It will certainly save developers' time and effort.

[hoch]

@joeberkovitz and @padenot: can we reevaluate the idea of using Mixin pattern to resolve this issue? As I described in the comment above, the introduction of BaseAudioContext is one step forward in improving the extensibility of the API. For instance, we might want to introduce another type of AudioContext into the API in the future - this mixin pattern will just create another stand alone context object.

@foolip WDYT?

[foolip]

From what I found in #1022 (comment), breakage should be fairly small, so if the WG participants think a real BaseAudioContext will be better in the long term, it sounds worth trying. I would make sure to look at each result from http archive individually and reach out to the original libraries first, so that the guidance when it breaks is simple.

Just to have all options on the table, simply going back to having OfflineAudioContext inherit from AudioContext ought to also work, by making the things that don't make sense on OfflineAudioContext throw.

[hoch]

@joeberkovitz, @mdjp and @padenot: Can we have a decision on this? We have 3 options.

1. Remove BaseAudioContext and go back to the original inheritance.
2. Use mixin pattern.
3. Keep BaseAudioContext.

@rtoy and I are in favor of the option 3. I believe the logic and justification are discussed enough in this thread.

[joeberkovitz]

@hoch I never felt super strongly about this. Given that breakage would be small, option 3 seems good to me.

[hoch]

@joeberkovitz Thanks for your input!

Since the breakage (even if it is small) is out there, we really need to make our decision ASAP.

[rtoy]

Friendly ping again. To reiterate, I am strongly in favor of option 3. It's a very clean view, makes the relationship between AudioContext and OfflineAudioContext clear, and makes for a nice base class on which to extend the context to the future.

When last counted there were less than a dozen sites broken---presumably a veru tiny fraction of the 0.5% of all pages using WebAudio.. We've already reached out to some of these sites about the issue and some have been fixed.

[padenot]

Let's do option 3.

[hoch]

Thank you all for your response! Glad we get to keep this change.

I will follow up with some compat analysis and also send out the intent to make this change official in Chrome.

[rtoy]

Closing this issue. We intend to keep BaseAudioContext