Closed Bug 830992 Opened 9 years ago Closed 9 years ago

Switch DOMTransaction to WebIDL codegen

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 3 obsolete files)

Once bug 822470 lands.
Attachment #702644 - Flags: review?(wchen)
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...
In fact, I'm going to switch to doing that.
Attachment #702643 - Attachment is obsolete: true
Attachment #702643 - Flags: review?(peterv)
Attachment #702854 - Flags: review?(wchen)
Attachment #702644 - Attachment is obsolete: true
Attachment #702644 - Flags: review?(wchen)
Whiteboard: [need review]
Attachment #702853 - Flags: review?(peterv) → review+
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.
Attachment #702854 - Flags: review?(wchen)
Attachment #702854 - Flags: review?(peterv)
Attachment #702854 - Flags: review+
>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.
Attachment #707796 - Flags: review?(wchen)
Attachment #707796 - Flags: review?(peterv)
Attachment #702854 - Attachment is obsolete: true
Attachment #702854 - Flags: review?(peterv)
Attachment #707796 - Flags: review?(wchen) → review+
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?
Attachment #707796 - Flags: review?(peterv) → review+
> 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
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla21
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.