Closed Bug 977287 Opened 6 years ago Closed 6 years ago

Rename the operation callback

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

"operation callback" is meaningless. It should be called the interrupt callback.
Riskiest thing going on here is moving js_HandleExecutionInterrupt into a namespace and removing the `extern` keyword from its declaration.
Assignee: general → jorendorff
Attachment #8383355 - Flags: review?(luke)
Not-quite-as-trivial odds and ends found during renaming.

For example, it looks like JSShellContextData::startTime isn't used from the interrupt callback anymore, for years now.
Attachment #8383357 - Flags: review?(luke)
I'd love to delete the following paragraph from jsapi.h. It basically says, "Note: we might call this callback randomly. No reason. We just do sometimes." Maybe I'm the only person bothered by this.

> /*
>  * These functions allow setting an interrupt callback that will be called
>  * from the JS thread some time after any thread triggered the callback using
>  * JS_RequestInterruptCallback(rt).
>  *
>- * To schedule the GC and for other activities the engine internally triggers
>- * interrupt callbacks. The embedding should thus not rely on callbacks being
>- * triggered through the external API only.
>- *
>  * Important note: Additional callbacks can occur inside the callback handler
>  * if it re-enters the JS engine. The embedding must ensure that the callback
>  * is disconnected before attempting such re-entry.
>  */

It would be an easy fix, since all the interrupt state is already protected by a lock. We'd still need a comment to the effect that interrupts can be coalesced; you might request 2 and only get 1.
Comment on attachment 8383355 [details] [diff] [review]
bug-977287-part-1-renaming-v1.patch

Lovely
Attachment #8383355 - Flags: review?(luke) → review+
Attachment #8383357 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/53139214dcf3
https://hg.mozilla.org/mozilla-central/rev/02400c717fa6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.