Convert nsISHistoryInternal's transaction list to an nsTArray

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

4 months ago
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).
(Assignee)

Comment 1

4 months ago
Created attachment 9004773 [details] [diff] [review]
Fix nsISHTransaction.idl's formatting

By replacing tabs with spaces, and cleaning up the comments slightly.
Attachment #9004773 - Flags: review?(nika)
(Assignee)

Updated

4 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 months ago
Created attachment 9004774 [details] [diff] [review]
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.
Attachment #9004774 - Flags: review?(nika)
(Assignee)

Comment 3

4 months ago
Created attachment 9004775 [details] [diff] [review]
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.)
Attachment #9004775 - Flags: review?(nika)
(Assignee)

Comment 4

4 months ago
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.

Updated

4 months ago
Priority: -- → P2

Comment 5

4 months ago
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 6

4 months ago
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+

Updated

4 months ago
Attachment #9004775 - Flags: review?(nika) → review+
(Assignee)

Comment 7

4 months ago
> 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

4 months ago
Pushed by nnethercote@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/caf7b122907d
https://hg.mozilla.org/mozilla-central/rev/e39b65db5b40
https://hg.mozilla.org/mozilla-central/rev/4ec8618d4fba
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Comment 10

4 months ago
Created attachment 9005464 [details] [diff] [review]
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.
Attachment #9005464 - Flags: review?(nika)
Depends on: 1487541

Comment 11

4 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bea6259e3a50
Mark GetTransactionAtIndex() as NS_IMETHODIMP. r=me
(Assignee)

Comment 12

4 months ago
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+
You need to log in before you can comment on or make changes to this bug.