Convert the shistory to use mozilla/LinkedList.h rather than prclist.h

RESOLVED FIXED in Firefox 55

Status

()

Core
Document Navigation
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: erahm, Assigned: Paul Bignier, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=c++] [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

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
Mentor: erahm@mozilla.com
Whiteboard: [lang=c++] [good first bug]
(Assignee)

Comment 1

8 months ago
Hi Eric,
I would like to take this on!
Kind regards, Paul
(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

8 months ago
Created attachment 8858861 [details] [diff] [review]
Convert the shistory to use mozilla/LinkedList.h rather than prclist.h.
(Assignee)

Updated

8 months ago
Attachment #8858861 - Flags: review?(erahm)
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

8 months ago
Created attachment 8859078 [details] [diff] [review]
Convert the shistory to use mozilla/LinkedList.h rather than prclist.h.
(Assignee)

Comment 6

8 months 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)
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

8 months ago
Attachment #8858861 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8859078 - Attachment is obsolete: true
(Assignee)

Comment 8

8 months ago
Created attachment 8859661 [details] [diff] [review]
Convert the shistory to use mozilla/LinkedList.h rather than prclist.h.
Attachment #8859661 - Flags: review?(erahm)
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 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+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6de4d3b055b620f988ecd5e8d7f0efa51fb3c8f
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ea6a7c6f2a5
Status: NEW → RESOLVED
Last Resolved: 8 months 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.