1486358 - Convert nsISHistoryInternal's transaction list to an nsTArray

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Nicholas Nethercote [inactive]]

nsISHistoryInternal has a list of nsISHTransaction objects. nsISHistoryInternal.rootTransaction is the start of the list, and nsISHTransaction has `prev` and `next` members. We also maintain indices into the list.

This bug will change the list to an nsTArray, which will make everything much simpler. The list has starting max of only 50 elements so performance should not be an issue (and the nsTArray might be faster in a lot of cases anyway).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix nsISHTransaction.idl's formatting

By replacing tabs with spaces, and cleaning up the comments slightly.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Change nsISHistory's transactions list to an nsTArray

- nsISHistoryInternal loses `rootTransaction`, and gains
  GetTransactionAtIndex(). (The implementing class `nsSHistory` already
  implemented a method by that method.)

- nsSHistory loses `mListRoot` in favour of `mTransactions`. It also loses
  `mLength`, because `mTransactions` tracks the length itself.

- nsISHTransaction.{prev,next} are no longer needed.

- nsISHTransaction.create() is no longer needed, because all it does now is set
  the SHEntry, and we can use SetSHEntry() for that.

Overall this deletes about 200 lines of code.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Factor out shistory entry window code

(It's worth noting that I had to fix one of these instances in the previous
patch, by adding a -1.)

Comment 4

[Nicholas Nethercote [inactive]]

I'm getting a bunch of test failures from the second patch:

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

I haven't managed to figure out the problem yet, though I expect/hope it's something small. Nika, maybe I'll get lucky and you'll spot something in your review.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9004773 [details] [diff] [review]
Fix nsISHTransaction.idl's formatting

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

::: docshell/shistory/nsISHTransaction.idl
@@ +11,4 @@
>  [scriptable, uuid(2EDF705F-D252-4971-9F09-71DD0F760DC6)]
>  interface nsISHTransaction : nsISupports
>  {
> +    /**

If you're re-indenting this file, can we switch it to 2-spaces?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9004774 [details] [diff] [review]
Change nsISHistory's transactions list to an nsTArray

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

::: toolkit/modules/sessionstore/SessionHistory.jsm
@@ +83,5 @@
>      if (history && history.count > 0) {
> +      // Loop over the transactions so we can get the persist property for each
> +      // one.
> +      let shistory = history.legacySHistory.QueryInterface(Ci.nsISHistory);
> +      let shistoryInternal = history.legacySHistory.QueryInterface(Ci.nsISHistoryInternal);

I think it would be reasonable to eliminate the difference between nsISHistory and nsISHistoryInternal in a follow-up. We don't have addons anymore.

Comment 7

[Nicholas Nethercote [inactive]]

> I haven't managed to figure out the problem yet, though I expect/hope it's something small.

I should've guessed: the problem was in the JS code. I broke how `entryCount` works in that loop.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caf7b122907d
Fix nsISHTransaction.idl's formatting.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e39b65db5b40
Change nsISHistory's transactions list to an nsTArray. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec8618d4fba
Factor out shistory entry window code. r=nika

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/caf7b122907d
https://hg.mozilla.org/mozilla-central/rev/e39b65db5b40
https://hg.mozilla.org/mozilla-central/rev/4ec8618d4fba

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Mark GetTransactionAtIndex() as NS_IMETHODIMP

I'm surprised the Windows builds aren't already complaining about this, because
`NS_IMETHODIMP` is different to `nsresult` on Windows.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bea6259e3a50
Mark GetTransactionAtIndex() as NS_IMETHODIMP. r=me

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 9005464 [details] [diff] [review]
Mark GetTransactionAtIndex() as NS_IMETHODIMP

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

I am approving this myself because it's trivial and I want it in along with the patch in bug 1487541.

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/bea6259e3a50