830992 - Switch DOMTransaction to WebIDL codegen

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Once bug 822470 lands.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it possible to pass CallbackObject& as the thisObj for callback functions.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch DOMTransaction to WebIDL codegen.

Comment 3

[Boris Zbarsky [:bzbarsky]]

A note on part 1: The other option is to keep taking a pointer type but add something to WrapCallThisObject that calls a function that might return a JSObject.  This function would have two overloads, one for CallbackObject* and one for void*, with the latter always returning null.  If the function returns null, we would go ahead with WrapNativeParent-type stuff.

That would perhaps be better because it wouldn't involve consumers trying to WrapNativeParent a CallbackObject* by accident because they forgot to dereference it before passing in...

Comment 4

[Boris Zbarsky [:bzbarsky]]

In fact, I'm going to switch to doing that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with new simpler non-SFINAE-happy setup.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to new thisobj setup

Comment 7

[William Chen [:wchen]]

Comment on attachment 702854 [details] [diff] [review]
Part 2 updated to new thisobj setup

Review of attachment 702854 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the mentioned changes.

peterv: can you review Codegen.py?

::: content/html/content/src/UndoManager.cpp
@@ +759,5 @@
> +  nsRefPtr<DOMTransactionCallback> redo = mTransaction->GetRedo(rv);
> +  if (!rv.Failed() && redo) {
> +    redo->Call(mTransaction.get(), rv);
> +  }
> +  // Should we ignore rv?

Yes, we ignore the rv because we want to avoid the rollback behavior of the nsITransactionManager.

@@ +776,5 @@
> +  nsRefPtr<DOMTransactionCallback> undo = mTransaction->GetUndo(rv);
> +  if (!rv.Failed() && undo) {
> +    undo->Call(mTransaction.get(), rv);
> +  }
> +  // Should we ignore rv?

Same as above.

@@ +840,5 @@
>  
>    TxnScopeGuard guard(this);
>  
>    // First try executing an automatic transaction.
> +  

Nit: Trailing whitespace.

@@ +845,2 @@
>  
> +  if (!AutomaticTransact(&aTransaction, aRv)) {

When I wrote this I wanted the behavior to branch on whether the "executeAutomatic" function was defined or not, however with XPIDL I had to try and call the function to find out. With WebIDL we can simply interrogate whether "executeAutomatic" is defined so it would be better to refactor it like this:

nsRefPtr<DOMTransactionCallback> executeAutomatic = aTransaction->GetExecuteAutomatic(aRv);
if (!aRv.Failed() && executeAutomatic) {
  AutomaticTransact(aTransaction, aRv);
} else {
  ManualTransaction(aTransaction, aRv);
}

@@ +888,5 @@
>    mTxnManager->BeginBatch(aTransaction);
>    mTxnManager->DoTransaction(undoTxn);
>    mHostNode->AddMutationObserver(mutationObserver);
>  
> +  bool retval = true;

We don't need this with the changes mentioned above.

::: content/html/content/src/UndoManager.h
@@ +75,5 @@
>  
>    /**
>     * Executes |aTransaction| as an automatic transaction.
> +   *
> +   * Returns false if we should try a manual transaction instead.

Not needed with the changes mentioned above.

Comment 8

[Boris Zbarsky [:bzbarsky]]

>nsRefPtr<DOMTransactionCallback> executeAutomatic = aTransaction->GetExecuteAutomatic(aRv);
>if (!aRv.Failed() && executeAutomatic) {
>  AutomaticTransact(aTransaction, aRv);

Hmm.  That doesn't work right, because it calls GetExecuteAutomatic twice.  But I can make something like this that will work right, I guess.

I'll post an updated patch with that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing the review comments

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 707796 [details] [diff] [review]
Updated to review comments

Review of attachment 707796 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/UndoManager.cpp
@@ +874,5 @@
>  }
>  
>  void
> +UndoManager::AutomaticTransact(DOMTransaction* aTransaction,
> +			       DOMTransactionCallback* aCallback,

There seems to be a tab here?

@@ +1162,4 @@
>    nsCOMArray<nsIVariant> keepAlive;
>    nsTArray<nsIVariant*> transactionItems;
>    for (uint32_t i = 0; i < items.Length(); i++) {
> +    jsval txVal = JS::ObjectValue(*items[i]->Callback());

JS::Value?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> There seems to be a tab here?

Indeed.  I'm adding a modeline to this file so this will stop happening.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bfa0b091b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdb0c716752

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8bfa0b091b5
https://hg.mozilla.org/mozilla-central/rev/fbdb0c716752