Closed
Bug 830992
Opened 10 years ago
Closed 10 years ago
Switch DOMTransaction to WebIDL codegen
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(3 files, 3 obsolete files)
2.27 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
20.62 KB,
patch
|
wchen
:
review+
peterv
:
review+
|
Details | Diff | Splinter Review |
5.21 KB,
patch
|
Details | Diff | Splinter Review |
Once bug 822470 lands.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
Attachment #702643 -
Flags: review?(peterv)
![]() |
Assignee | |
Comment 2•10 years ago
|
||
Attachment #702644 -
Flags: review?(wchen)
![]() |
Assignee | |
Comment 3•10 years ago
|
||
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...
![]() |
Assignee | |
Comment 4•10 years ago
|
||
In fact, I'm going to switch to doing that.
![]() |
Assignee | |
Comment 5•10 years ago
|
||
Attachment #702853 -
Flags: review?(peterv)
![]() |
Assignee | |
Updated•10 years ago
|
Attachment #702643 -
Attachment is obsolete: true
Attachment #702643 -
Flags: review?(peterv)
![]() |
Assignee | |
Comment 6•10 years ago
|
||
Attachment #702854 -
Flags: review?(wchen)
![]() |
Assignee | |
Updated•10 years ago
|
Attachment #702644 -
Attachment is obsolete: true
Attachment #702644 -
Flags: review?(wchen)
![]() |
Assignee | |
Updated•10 years ago
|
Whiteboard: [need review]
Updated•10 years ago
|
Attachment #702853 -
Flags: review?(peterv) → review+
Comment 7•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 8•10 years ago
|
||
>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.
![]() |
Assignee | |
Comment 9•10 years ago
|
||
Attachment #707796 -
Flags: review?(wchen)
Attachment #707796 -
Flags: review?(peterv)
![]() |
Assignee | |
Updated•10 years ago
|
Attachment #702854 -
Attachment is obsolete: true
Attachment #702854 -
Flags: review?(peterv)
![]() |
Assignee | |
Comment 10•10 years ago
|
||
Updated•10 years ago
|
Attachment #707796 -
Flags: review?(wchen) → review+
Comment 11•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 12•10 years ago
|
||
> 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
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a8bfa0b091b5 https://hg.mozilla.org/mozilla-central/rev/fbdb0c716752
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•