Closed Bug 461212 Opened 13 years ago Closed 13 years ago

deCOM frame traversal (nsIFrameEnumerator)


(Core :: Layout, defect)

Not set





(Reporter: benjamin, Assigned: benjamin)




(1 file, 1 obsolete file)

Attached 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)".
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.
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
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+
and stupid bustage fix

Followup bug filed as 461919
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 461938
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 → ---
Closed: 13 years ago13 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.