deCOM frame traversal (nsIFrameEnumerator)

RESOLVED FIXED in mozilla1.9.1b2



11 years ago
11 years ago


(Reporter: benjamin, Assigned: benjamin)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



11 years ago
Posted patch deCOM frame traversal, rev. 1 (obsolete) — Splinter Review
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.
Attachment #344342 - Flags: superreview?(roc)
Attachment #344342 - Flags: review?(roc)
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)".

Comment 3

11 years ago
Yeah, that would be great. Can I do that in a separate followup?
Attachment #344342 - Flags: superreview?(roc)
Attachment #344342 - Flags: superreview+
Attachment #344342 - Flags: review?(roc)
Attachment #344342 - Flags: review+
This looks like a change in behaviour:

I think we should still break out of the loop, as the comment suggests,
when Prev() fails.

And in the next hunk:

The old code is careful not to assign 'resultFrame' so if Next() fails
'aPos->mResultFrame' will be assigned its current value.

Comment 6

11 years ago
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.

Comment 8

11 years ago
Updated per comments
Attachment #344342 - Attachment is obsolete: true
Attachment #344677 - Flags: review?(mats.palmgren)
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.
Attachment #344677 - Flags: review?(mats.palmgren) → review+

Comment 10

11 years ago
and stupid bustage fix

Followup bug filed as 461919
Last Resolved: 11 years ago
Resolution: --- → FIXED


11 years ago
Blocks: 461938

Comment 11

11 years ago
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
Resolution: FIXED → ---

Comment 12

11 years ago
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.