1322920 - Remove old DOM Promise implementation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Till Schneidereit [:till]]

There's really no way for us to move back to the old Promise implementation by now, if only because async/await would have to be disabled. Let's remove it.

30 files changed, 1235 insertions(+), 5204 deletions(-)

Comment 1

[Till Schneidereit [:till]]

Attachment: Bug 1322920 - Remove DOM Promise implementation.

Review commit: https://reviewboard.mozilla.org/r/98088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98088/

Comment 2

[Till Schneidereit [:till]]

Boris, given that you introduced almost all of the ifdef's this removes, it'd be great if you could do the review. The js/src parts are very straight-forward, but I'm happy to ask someone else for those, too.

I think there's more that could be done in Codegen.py, but this works and simplifies a lot of things.

Also, the diffstats posted in comment 0 overstate the number of inserts drastically. The truth is closer to what mozreview thinks: +9 / -4.0k

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817906 [details]
Bug 1322920 - Remove DOM Promise implementation.

https://reviewboard.mozilla.org/r/98088/#review98358

Thank you for cleaning this up!

There's definitely more stuff in Codegen.py that can go away.  Please file a followup; feel free to assign to me if you don't have the time or the inclination to deal with it.

::: dom/promise/Promise.h:43
(Diff revision 1)
> -                public nsWrapperCache,
> -#endif // SPIDERMONKEY_PROMISE
>                  public SupportsWeakPtr<Promise>
>  {
> -  friend class NativePromiseCallback;
>    friend class PromiseReactionJob;

Is this still non-dead-code?  I think it's dead, since you're removing the actual class definition in the .cpp file.

In general, there might well be more dead code here.  Please file a followup to check; again feel free to assign to me.

::: dom/promise/Promise.h:47
(Diff revision 1)
> -#endif // defined(DOM_PROMISE_DEPRECATED_REPORTING)
>    friend class PromiseWorkerProxy;
>    friend class PromiseWorkerProxyRunnable;
> -  friend class RejectPromiseCallback;
> -  friend class ResolvePromiseCallback;
>    friend class PromiseResolveThenableJob;

Pretty sure this is dead too.

::: dom/promise/Promise.h:52
(Diff revision 1)
>  
>  public:
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_PROMISE_IID)
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> -#ifdef SPIDERMONKEY_PROMISE
>    // We're not skippable, since we're not owned from JS to start with.

This comment can probably go away, since "not skippable" is the normal state of things.  It was just there to explain the difference from the non-spidermonkey-promise case, which was confusing without any explanation.

::: dom/promise/Promise.cpp:644
(Diff revision 1)
>      }
>      context->AfterProcessMicrotask();
>    }
>  }
>  
> -#ifndef SPIDERMONKEY_PROMISE
> +JSObject*

Man, mozreview really got confused here and made this diff _very_ hard to read.  Please file a bug on it.  The raw diff is much saner!

::: dom/webidl/Promise.webidl:20
(Diff revision 1)
>  callback PromiseJobCallback = void();
>  
>  [TreatNonCallableAsNull]
>  callback AnyCallback = any (any value);
>  
> -// When using SpiderMonkey promises, we don't want to define all this stuff;
> +// Promises are implemented in SpiderMonkey, just define a tiny interface to make

';', not ',', before "just".

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Please file a bug on it.

I filed bug 1323011.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8817906 [details]
Bug 1322920 - Remove DOM Promise implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98088/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/918e37b44bdd
Remove DOM Promise implementation. r=bz

Comment 7

[Till Schneidereit [:till]]

Remote:
https://hg.mozilla.org/integration/mozilla-inbound/rev/918e37b44bddf0ab04f59887a1bb753aa586b3d9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/918e37b44bdd

Comment 9

[Boris Zbarsky [:bzbarsky]]

Till, did you end up filing a bug on the codegen followup bigs?

Comment 10

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> Till, did you end up filing a bug on the codegen followup bigs?

I hadn't yet, but it was on my todo list - I filed bug 1323721. Thanks for the reminder.