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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Paul Bignier]

Hi Eric,
I would like to take this on!
Kind regards, Paul

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

Comment 3

[Paul Bignier]

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

Comment 4

[Eric Rahm [: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;

Comment 5

[Paul Bignier]

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

Comment 6

[Paul Bignier]

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

Comment 7

[Eric Rahm [: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

Comment 8

[Paul Bignier]

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

Comment 9

[Eric Rahm [: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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8859661 [details] [diff] [review]
Convert the shistory to use mozilla/LinkedList.h rather than prclist.h.

r=me

Comment 11

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6de4d3b055b620f988ecd5e8d7f0efa51fb3c8f

Comment 12

[Eric Rahm [:erahm]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ea6a7c6f2a5