Last Comment Bug 771435 - Fix nsEditor::DoTransaction to use transaction interfaces sanely
: Fix nsEditor::DoTransaction to use transaction interfaces sanely
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (2.10 KB, patch)
2012-07-08 05:34 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | 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;
    // 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 (busy, don't ask for review please) 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]

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!

Comment 3 :Aryeh Gregor (away until August 15) 2012-07-10 00:47:57 PDT
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:47:15 PDT

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