Frame traversal currently uses nsIBidirectionalEnumerator. I need to use a custom enumerator class when nsIFrame stops inheriting from nsISupports, and there's some nice de-COM that can happen at the same time. When reviewing, let me know if you think this is something that we could/should take in 1.9.1 (probably not), or should wait until after branching.

Comment on attachment 344342 [details] [diff] [review] deCOM frame traversal, rev. 1 Not a real review, just something I happened to notice: >@@ -5477,43 +5468,37 @@ nsIFrame::GetFrameFromDirection(nsDirect ... > > if (aDirection == eDirNext) >- result = frameTraversal->Next(); >+ frameTraversal->Next(); > else >- result = frameTraversal->Prev(); >+ frameTraversal->Prev(); > if (NS_FAILED(result)) > return result; You should get rid of this result check now.

Hmm. If only we could get rid of the use from nsTypeAheadFind, we could go ahead and completely devirtualize it so it's not an XPCOM component anymore. The NewFrameTraversal function would just become a constructor on the object. One option would be to replace the complete while loop in nsTypeAheadFind with a single function in nsIPresShell, say "GetNextVisibleFrame(nsIFrame* aStartFrame, nscoord aMinimumVisibility)".

Yeah, that would be great. Can I do that in a separate followup?

Comment on attachment 344342 [details] [diff] [review] deCOM frame traversal, rev. 1 OK

This looks like a change in behaviour: https://bugzilla.mozilla.org/attachment.cgi?id=344342&action=diff#a/layout/generic/nsFrame.cpp_sec2 I think we should still break out of the loop, as the comment suggests, when Prev() fails. And in the next hunk: https://bugzilla.mozilla.org/attachment.cgi?id=344342&action=diff#a/layout/generic/nsFrame.cpp_sec3 The old code is careful not to assign 'resultFrame' so if Next() fails 'aPos->mResultFrame' will be assigned its current value.

Prev() never fails (the new signature returns void). The second comment is valid, I'll have a new patch up shortly.

Ok, I still find the comment in the first hunk slightly misleading because it suggests "Prev(); if (!CurrentItem()) break;" and I wonder if that's not what the author originally intended, although it's not what the code does currently.

Updated per comments

Comment on attachment 344677 [details] [diff] [review] deCOM frame traversal, rev. 1.1 r=mats with a few nits: > nsEventStateManager.cpp > + while (true) { Hmm, are we allowed to use C++ bool/true/false now? > nsFrameTraversal.cpp > + nsRefPtr<nsFrameIterator> trav; Can you use "nsCOMPtr<nsIFrameEnumerator> trav" instead to avoid instantiating that template? (it's not used anywhere else AFAICT) > nsFrameIterator::IsDone()//what does this mean??off edge? yes You removed that comment in the declaration, I think you should remove it here too. > //always try previous on THAT line if that fails go the other way I think this comment is misleading, see comment 7. Please file a followup bug to have it fixed one way or the other.

http://hg.mozilla.org/mozilla-central/rev/affcc1c08bc0 and stupid bustage fix http://hg.mozilla.org/mozilla-central/rev/eda9709dc586 Followup bug filed as 461919

Backed out: mochitest regressions from this or from bug 461410; also seamonkey build bustage because they're using the old CVS typeaheadfind code, which I've filed as bug 461938

relanded: http://hg.mozilla.org/mozilla-central/rev/307153dd899d