Last Comment Bug 771435 - Fix nsEditor::DoTransaction to use transaction interfaces sanely
: Fix nsEditor::DoTransaction to use transaction interfaces sanely
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 489851 765595
  Show dependency treegraph
 
Reported: 2012-07-06 00:34 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-07-10 15:47 PDT (History)
1 user (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.10 KB, patch)
2012-07-08 05:34 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-07-06 00:34:01 PDT
From nsEditor::DoTransaction:

    // We start off with an EditTxn since that's what the factory returns.
    nsRefPtr<EditTxn> editTxn = new PlaceholderTxn();

    // Then we QI to an nsIAbsorbingTransaction to get at placeholder
    // functionality
    nsCOMPtr<nsIAbsorbingTransaction> plcTxn;
    editTxn->QueryInterface(NS_GET_IID(nsIAbsorbingTransaction),
                            getter_AddRefs(plcTxn));
    // have to use line above instead of "plcTxn = do_QueryInterface(editTxn);"
    // due to our broken interface model for transactions.

    // save off weak reference to placeholder txn
    mPlaceHolderTxn = do_GetWeakReference(plcTxn);

editTxn is not used thereafter, only plcTxn.  This is outdated -- bug 489851 got rid of the factory functions that the comments refer to, so we could just use nsRefPtr<PlaceholderTxn> here, which requires no QI to be an nsIAbsorbingTransaction.  But then do_GetWeakReference doesn't work.  What's the right way to work around this?
Comment 1 :Ehsan Akhgari 2012-07-06 09:48:07 PDT
Hmm, how about:

nsCOMPtr<nsIAbsorbingTransaction> plcTxn = new PlaceholderTxn();

You have the concrete type here, so there's no reason to QI, and the compiler can figure things out using a static cast.
Comment 2 :Aryeh Gregor (away until August 15) 2012-07-08 05:34:28 PDT
Created attachment 640058 [details] [diff] [review]
Patch

Hmm, that works.  We still need to QI it to nsITransaction, which isn't maximally elegant, but it's still a lot better.  -8 lines of comments complaining about how bad the code is!

Try: https://tbpl.mozilla.org/?tree=Try&rev=c4a58cf9f185
Comment 3 :Aryeh Gregor (away until August 15) 2012-07-10 00:47:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b855bdde1a9
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:47:15 PDT
https://hg.mozilla.org/mozilla-central/rev/5b855bdde1a9

Note You need to log in before you can comment on or make changes to this bug.