Closed Bug 1322920 Opened 3 years ago Closed 3 years ago

Remove old DOM Promise implementation

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: till, Assigned: till)

References

Details

Attachments

(1 file)

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(-)
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
Flags: needinfo?(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".
Attachment #8817906 - Flags: review+
Flags: needinfo?(bzbarsky)
> Please file a bug on it.

I filed bug 1323011.
https://hg.mozilla.org/mozilla-central/rev/918e37b44bdd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Till, did you end up filing a bug on the codegen followup bigs?
Flags: needinfo?(till)
Blocks: 1323721
(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.
Flags: needinfo?(till)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.