Closed Bug 1486358 Opened 2 years ago Closed 2 years ago

Convert nsISHistoryInternal's transaction list to an nsTArray


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




Tracking Status
firefox63 --- fixed


(Reporter: njn, Assigned: njn)




(4 files)

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).
By replacing tabs with spaces, and cleaning up the comments slightly.
Attachment #9004773 - Flags: review?(nika)
Assignee: nobody → n.nethercote
- 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.
Attachment #9004774 - Flags: review?(nika)
(It's worth noting that I had to fix one of these instances in the previous
patch, by adding a -1.)
Attachment #9004775 - Flags: review?(nika)
I'm getting a bunch of test failures from the second patch:

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.
Priority: -- → P2
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?
Attachment #9004773 - Flags: review?(nika) → review+
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.
Attachment #9004774 - Flags: review?(nika) → review+
Attachment #9004775 - Flags: review?(nika) → review+
> 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.
Pushed by
Fix nsISHTransaction.idl's formatting.
Change nsISHistory's transactions list to an nsTArray. r=nika
Factor out shistory entry window code. r=nika
I'm surprised the Windows builds aren't already complaining about this, because
`NS_IMETHODIMP` is different to `nsresult` on Windows.
Attachment #9005464 - Flags: review?(nika)
Depends on: 1487541
Pushed by
Mark GetTransactionAtIndex() as NS_IMETHODIMP. r=me
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.
Attachment #9005464 - Flags: review?(nika) → review+
Depends on: 1524359
You need to log in before you can comment on or make changes to this bug.