Closed Bug 1322920 Opened 3 years ago Closed 3 years ago

Remove old DOM Promise implementation


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

Not set



Tracking Status
firefox53 --- fixed


(Reporter: till, Assigned: till)




(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, 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.

Thank you for cleaning this up!

There's definitely more stuff in 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,
>                  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)
>    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:
>    // 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();
>    }
>  }
> +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.
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.