Closed Bug 1143513 Opened 9 years ago Closed 9 years ago

Implement nsFrameList::Iterator to make nsFrameList compatible with range-based syntax and utils

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files, 3 obsolete files)

They might eventually replace nsFrameList::Slice and Enumerator. But in this bug, I'm only going to implement them without replacing any existing usage of the old classes.
No longer blocks: 1141931
Looking forward to it. :-)  Do you have a proposal for how these will work?

I'd prefer frame sibling iteration to be "open ended" by default, i.e. where
GetNextSibling() == nullptr is the end condition.  Although a "fixed" version
is likely useful as well.  'Slice' can be either, which adds complexity to
the code and ambiguity for the reader.  It's probably better to have separate
types for these two cases.
It seems that a linked-list with nullptr as sentry like nsFrameList is not compatible with reverse iterator. That was the reason I abandoned fixing this before bug 1141931.

I guess we probably have to keep the reference to the nsFrameList in the range and iterator to make everything work. It doesn't seem to be a big problem, though. I'll try that way later.
Summary: Implement nsFrameList::Range & Iterator to make nsFrameList compatible with range-based syntax and utils → Implement nsFrameList::Iterator to make nsFrameList compatible with range-based syntax and utils
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Attachment #8579127 - Flags: review?(roc)
Attachment #8579127 - Flags: review?(jwalden+bmo)
Attached patch usage example (obsolete) — Splinter Review
Comment on attachment 8579127 [details] [diff] [review]
patch

Review of attachment 8579127 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrameList.h
@@ +451,5 @@
> +  class Iterator
> +  {
> +  public:
> +    typedef nsIFrame ValueType;
> +    // Though we don't support +/- a integer, but iterators

delete "but"

@@ +465,5 @@
> +      : mList(aOther.mList)
> +      , mCurrent(aOther.mCurrent)
> +    {}
> +
> +    ValueType& operator*() const { return *mCurrent; }

We rarely use nsIFrame& references in layout. Maybe this should return nsIFrame*?
Attachment #8579127 - Flags: review?(roc)
I'd prefer ValueType to be nsIFrame* too.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 8579127 [details] [diff] [review]
> patch
> 
> Review of attachment 8579127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsFrameList.h
> @@ +451,5 @@
> > +  class Iterator
> > +  {
> > +  public:
> > +    typedef nsIFrame ValueType;
> > +    // Though we don't support +/- a integer, but iterators
> 
> delete "but"

Ah... Sometimes I forgot the words do not appear together in English :)

> @@ +465,5 @@
> > +      : mList(aOther.mList)
> > +      , mCurrent(aOther.mCurrent)
> > +    {}
> > +
> > +    ValueType& operator*() const { return *mCurrent; }
> 
> We rarely use nsIFrame& references in layout. Maybe this should return
> nsIFrame*?

The ValueType could be nsIFrame*, but this operator still needs to return a reference anyway. So maybe "return &mCurrent;" just like what I have done for IntegerRange.

Any other comments?
Attached patch patch (obsolete) — Splinter Review
Attachment #8579127 - Attachment is obsolete: true
Attachment #8579127 - Flags: review?(jwalden+bmo)
Attachment #8579296 - Flags: review?(roc)
Attachment #8579296 - Flags: review?(jwalden+bmo)
Attached patch usage exampleSplinter Review
Attachment #8579128 - Attachment is obsolete: true
Why does it have to be a reference type?

This seems to work fine for me:
  const ValueType operator*() const { return mCurrent; }
Comment on attachment 8579296 [details] [diff] [review]
patch

Review of attachment 8579296 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with or without what Mats said
Attachment #8579296 - Flags: review?(roc) → review+
(In reply to Mats Palmgren (:mats) from comment #10)
> Why does it have to be a reference type?
> 
> This seems to work fine for me:
>   const ValueType operator*() const { return mCurrent; }

The problem is that it won't be compatible with ReverseIterator. But it seems even I make it reference type, it still cannot be use with that. I guess I have to change ReverseIterator to make it more general.
Attached patch patch, r=rocSplinter Review
It is compatible now without any change to ReverseIterator.
Attachment #8579296 - Attachment is obsolete: true
Attachment #8579296 - Flags: review?(jwalden+bmo)
Attachment #8579798 - Flags: review?(jwalden+bmo)
Waldo, is there anything blocks you reviewing this patch?
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8579798 [details] [diff] [review]
patch, r=roc

Review of attachment 8579798 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrameList.h
@@ +524,5 @@
> +operator==(const nsFrameList::Iterator& aIter1,
> +           const nsFrameList::Iterator& aIter2)
> +{
> +  MOZ_ASSERT(&aIter1.mList == &aIter2.mList,
> +             "Must not compare iterator from different list");

Assertion message formatting says this should start with a lowercase letter.
Attachment #8579798 - Flags: review?(jwalden+bmo) → review+
...and no, not quite anything blocking me reviewing.  Nothing but the rest of the goals, and the forty requests in my queue.  :-(
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/530a769b0dbd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: