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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(2 files, 3 obsolete files)
2.32 KB,
patch
|
Details | Diff | Splinter Review | |
5.01 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 3•9 years ago
|
||
Assignee: nobody → quanxunzhen
Attachment #8579127 -
Flags: review?(roc)
Attachment #8579127 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
I'd prefer ValueType to be nsIFrame* too.
Assignee | ||
Comment 7•9 years ago
|
||
(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?
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8579127 -
Attachment is obsolete: true
Attachment #8579127 -
Flags: review?(jwalden+bmo)
Attachment #8579296 -
Flags: review?(roc)
Attachment #8579296 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8579128 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
Waldo, is there anything blocks you reviewing this patch?
Flags: needinfo?(jwalden+bmo)
Comment 15•9 years ago
|
||
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+
Comment 16•9 years ago
|
||
...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)
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3210787928
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/530a769b0dbd
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/530a769b0dbd
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•