461212 - deCOM frame traversal (nsIFrameEnumerator)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Smedberg]

Attachment: deCOM frame traversal, rev. 1

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 1

[Uri Bernstein]

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)".

Comment 3

[Benjamin Smedberg]

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

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

OK

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Benjamin Smedberg]

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

Comment 7

[Mats Palmgren (inactive)]

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.

Comment 8

[Benjamin Smedberg]

Attachment: deCOM frame traversal, rev. 1.1

Updated per comments

Comment 9

[Mats Palmgren (inactive)]

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.

Comment 10

[Benjamin Smedberg]

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

Comment 11

[Benjamin Smedberg]

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

Comment 12

[Benjamin Smedberg]

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