Remove use of goog.Disposable()

[tschaub]

This replaces goog.Disposable() with a simpler ol.Disposable().

[tschaub]

Replacing the use of goog.dispose(obj) with obj.dispose() revealed that we were passing replay groups to goog.dispose() though they are not disposable. In addition, I notice that there is more cleaning up that the controls should do (unregistering listeners on DOM elements) in their disposeInternal methods - but that can be a separate effort.

[gberaudo]

Could it be annotated with @struct like ol.Observable?

[tschaub]

Done with 9b58672.

[tschaub]

Actually, marking as @struct is creating compiler issues with EventTarget (where it uses bracket notation). See https://travis-ci.org/openlayers/ol3/builds/113174215 for the failures.

[gberaudo]

Struct is needed for #4793 ; I can make the change after your PR is merged and I rebase on top of it.

[ahocevar]

This looks great @tschaub. I created a heap snapshot after running all tests from master and from your branch. And your branch results in a smaller heap with less DOM entries. So I'm in favour of merging this.

Regarding @gberaudo's comments, I'd not make ol.Disposable.prototype.dispose exportable. No dispose method is exportable now either. And I think it makes sense to keep ol.Disposable.prototype.disposed_ on the prototype, because it saves a bit of memory if the value is not set on every instance.

[tschaub]

I'm not liking this failure:

node tasks/test-rendering.js
Gecko: 40.0.3 
Script Error: [JavaScript Error: "TypeError: str is undefined" {file: "http://127.0.0.1:3001/node_modules/mocha/mocha.js" line: 5936}]
       Stack:
         -> http://127.0.0.1:3001/node_modules/mocha/mocha.js: 5936
1456939202517   addons.repository   WARN    cacheEnabled: Couldn't get pref: extensions.getAddons.cache.enabled
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

[tschaub]

Ok, the rendering test failure mentioned above was a real issue. But pretty badly buried.

The first issue is that when our afterEach test functions throw, we get an obscure error:

Uncaught TypeError: Cannot read property 'replace' of undefined
    exports.clean @ mocha.js:5937
    (anonymous function) @ mocha.js:2518
    EventEmitter.emit @ mocha.js:200
    (anonymous function) @ mocha.js:2451
    EventEmitter.emit @ mocha.js:200
    Runner.fail @ mocha.js:4564
    Runner.failHook @ mocha.js:4593
    (anonymous function) @ mocha.js:4638
    done @ mocha.js:4251
    Runnable.run @ mocha.js:4286
    next @ mocha.js:4629
    (anonymous function) @ mocha.js:4651
    timeslice @ mocha.js:12619

At the top of the stack, clean is getting called with undefined. Digging down, it looks like test.body should be defined, but it isn't. I didn't put together a test case to see if this is limited to our rendering tests or has something to do with the test runner, but it would be good to track that down. It means afterEach (and probably beforeEach) is a liability.

The real issue is that in the WebGL map renderer's disposeInternal method, it calls goog.dispose(this.context_). This is a no-op in master, because this function checks to see if the provided arg has a dispose method and does nothing if it doesn't.

But this convenient goog.dispose() function is masking the problem. The ol.webgl.Context thinks it is an event target, which should have a dispose method. And it implements disposeInternal, so someone thought this was going to be called. But nope, the ol.webgl.Context is missing the magic goog.inherits() call.

So ol.webgl.Context is not actually an ol.events.EventEmitter, and it doesn't have a dispose method (so its disposeInternal method is never getting called). And when goog.dispose(obj) is replaced with obj.dispose(), we get a failure. Which unfortunately is happening in a afterEach function that isn't handling errors as expected.

Anyway, it is not clear to me why ol.webgl.Context needs to be an event target. Maybe it should just be a disposable? Would be nice to have something that demonstrated (or tested) the expected functionality.

[tschaub]

It looks like ol.webgl.Context has been an event-target-that-isn't-an-event-target from the start. Anybody care to speculate about whether it should inherit from ol.events.EventTarget or ol.Disposable?

[ahocevar]

Anybody care to speculate about whether it should inherit from ol.events.EventTarget or ol.Disposable?

This is really just a speculation, but since it doesn't dispatch any events, I'd say it should inherit from ol.Disposable

[fredj]

+1 for ol.Disposable and a ol.events.unlistenAll(this.canvas_) must be added in ol.webgl.Context.prototype.disposeInternal

[tschaub]

This is really just a speculation, but since it doesn't dispatch any events, I'd say it should inherit from ol.Disposable

I've changed this in e1f0e8d.

a ol.events.unlistenAll(this.canvas_) must be added in ol.webgl.Context.prototype.disposeInternal

I've added this in 2f9d4e6.

Thanks for the reviews @gberaudo, @ahocevar, and @fredj. Does this look in good shape to everyone now?

[gberaudo]

LGTM, thanks.

[ahocevar]

Looks great. Thanks for the hard work @tschaub. Please merge.

[fredj]

+1