Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
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 (:ayg) (away until October 25)
: Jet Villegas (:jet)
Depends on:
Blocks: 489851 765595
  Show dependency treegraph
Reported: 2012-07-06 00:34 PDT by Aryeh Gregor (:ayg) (away until October 25)
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 (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 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 (Away Oct 25 - Nov 9) 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 (:ayg) (away until October 25) 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 (:ayg) (away until October 25) 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.