Closed
Bug 1356896
Opened 7 years ago
Closed 7 years ago
Convert the shistory to use mozilla/LinkedList.h rather than prclist.h
Categories
(Core :: DOM: Navigation, enhancement)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: erahm, Assigned: paul.bignier, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=c++] [good first bug])
Attachments
(1 file, 2 obsolete files)
5.26 KB,
patch
|
erahm
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We would like to convert the usage of |PRCList| to |mozilla::LinkedList| [1] in shistory. This is mainly: - make |nsSHistory| [2] inherit from |mozilla::LinkedListElement<nsSHistory>| rather than |PRCList|, updated it's constructor and destructor to use the LinkedListMethods - update |gSHistoryList| [3] to be a LinkedList, update it's usages [4] to use their LinkedList couterparts (pushBack, for x : list,etc) [1] http://searchfox.org/mozilla-central/source/mfbt/LinkedList.h [2] http://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/docshell/shistory/nsSHistory.h#27 [3] http://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/docshell/shistory/nsSHistory.cpp#52 [4] http://searchfox.org/mozilla-central/search?q=symbol:_ZL13gSHistoryList&redirect=false
Reporter | ||
Updated•7 years ago
|
Mentor: erahm
Whiteboard: [lang=c++] [good first bug]
Assignee | ||
Comment 1•7 years ago
|
||
Hi Eric, I would like to take this on! Kind regards, Paul
Comment 2•7 years ago
|
||
(In reply to Paul Bignier from comment #1) > Hi Eric, > I would like to take this on! > Kind regards, Paul Thank you Paul, assigning this to you :D
Assignee: nobody → paul.bignier
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8858861 -
Flags: review?(erahm)
Reporter | ||
Comment 4•7 years ago
|
||
Comment on attachment 8858861 [details] [diff] [review] Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. Review of attachment 8858861 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Paul, this looks good. I have a few comments on further simplifications we can do. ::: docshell/shistory/nsSHistory.cpp @@ +244,5 @@ > > nsSHistory::~nsSHistory() > { > // Remove this SHistory object from the list > + this->removeFrom(gSHistoryList); I think we can get rid of this entirely, LinkedListElements remove themselves from their list upon destruction [1]. [1] http://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/mfbt/LinkedList.h#195-199 @@ +1184,5 @@ > > nsTArray<TransactionAndDistance> transactions; > > + nsSHistory* shist = gSHistoryList.getFirst(); > + while (shist != nullptr) { This is correct, but we can simplify things further by using a for-in loop, ie: > for (auto shist : gHistoryList) { ... } ::: docshell/shistory/nsSHistory.h @@ +23,5 @@ > class nsSHistoryObserver; > class nsISHEntry; > class nsISHTransaction; > > +using namespace mozilla; Lets avoid including the whole namespace in the header, you can remove this and ... @@ +25,5 @@ > class nsISHTransaction; > > +using namespace mozilla; > + > +class nsSHistory final : public LinkedListElement<nsSHistory>, ... make this mozilla::LinkedListElement<nsSHistory> Another option would be to limit the scope of the using statement to just the part we care about: > using mozilla::LinkedList;
Attachment #8858861 -
Flags: review?(erahm) → feedback+
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8859078 [details] [diff] [review] Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. Thanks for the cool suggestions, it's all fixed! About the 'for' vs 'while', the intent is indeed clearer with a 'for' but I generally tend to minimize the diff in my patches. Regards, Paul
Attachment #8859078 -
Flags: review?(erahm)
Reporter | ||
Comment 7•7 years ago
|
||
Comment on attachment 8859078 [details] [diff] [review] Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. Review of attachment 8859078 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Just one more super small nit. ::: docshell/shistory/nsSHistory.cpp @@ +1181,5 @@ > // its SHistory's current index. > > nsTArray<TransactionAndDistance> transactions; > > + for (auto shist = gSHistoryList.getFirst(); shist != nullptr; shist = shist->getNext()) { LinkedList supports ranged-based for loops [1,2], so we can simplify this even further: > for (auto shist : gSHistoryList) [1] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/mfbt/LinkedList.h#532-542 [2] http://en.cppreference.com/w/cpp/language/range-for
Attachment #8859078 -
Flags: review?(erahm) → feedback+
Assignee | ||
Updated•7 years ago
|
Attachment #8858861 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8859078 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8859661 -
Flags: review?(erahm)
Reporter | ||
Comment 9•7 years ago
|
||
Comment on attachment 8859661 [details] [diff] [review] Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. Review of attachment 8859661 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Paul! Everything looks good to me, but I'm going to get a docshell peer to review this as well. In the meantime I'll push a try run. Hi Boris, this is pretty straightforward patch converting nsSHistory from PRCList over to mozilla::LinkedList. Everything looks good to me, but I'm not a peer here.
Attachment #8859661 -
Flags: review?(erahm)
Attachment #8859661 -
Flags: review?(bzbarsky)
Attachment #8859661 -
Flags: review+
Comment 10•7 years ago
|
||
Comment on attachment 8859661 [details] [diff] [review] Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. r=me
Attachment #8859661 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6de4d3b055b620f988ecd5e8d7f0efa51fb3c8f
Reporter | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea6a7c6f2a511e14b74656d68cab77cd35f46f9 Bug 1356896 - Convert the shistory to use mozilla/LinkedList.h rather than prclist.h. r=erahm
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ea6a7c6f2a5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•