1488321 - Remove nsISHTransaction

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

It's just a thin wrapper around nsISHEntry.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove `currentIndex` in nsSHistory::AddEntry()

It was used to protect against changes in mIndex by history listeners, but the
new code structure doesn't need it, because we want to use mIndex afterwards.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Make nsISHTransaction builtin and its attributes infallible

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Enforce non-nullness for nsISHTransaction.sHEntry

In practice it's always non-null, and only half the places that deal with it
have null handling code. So this simplifies things.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Improve GetTransactionAtIndex() result checking

Specifically:

- Check the nsresult consistently, but don't check (unnecessarily) for null on
  success.

- In AddEntry(), move GetTransactionAtIndex() before all state-changing
  operations, so we don't end up with partial state updates upon failure.

- Use early returns to reduce indenting.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Move the `persist` attribute from nsISHTransaction to nsISHEntry

This requires making nsISHEntry `builtinclass`.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Remove nsISHTransaction

Because it's a wafer-thin wrapper around nsISHEntry that just complicates
things.

This patch leaves behind a combination of "entry" and "transaction"
terminology. I'll fix that in a subsequent patch.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Fix up nsISHEntry.{index,getEntryAtIndex()}

nsISHEntry.index is readonly, but if you pass `true` as getEntryAtIndex()'s
second argument, nsISHEntry.index will be modified. This is pretty gross.

This patch changes `index` so it's not readonly (because it's not!) and removes
getEntryAtIndex()'s second argument.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Remove all traces of the "transaction" terminology in SHistory

This patch:

- removes GetTransactionAtIndex(), because getTransactionAtIndex() can be used
  instead;

- renames a lot of things;

- updates some comments.

Comment 9

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb1d304e7063e29e4820024ad49ceaa4fc618c49

Comment 10

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

Comment on attachment 9006135 [details] [diff] [review]
Make nsISHTransaction builtin and its attributes infallible

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

::: docshell/shistory/nsISHTransaction.idl
@@ +12,5 @@
>  {
>    /**
>     * The nsISHEntry for the current transaction.
>     */
> +  [infallible] attribute nsISHEntry sHEntry;

Huh, I completely forgot I had landed [infallible] on interface types!

::: docshell/shistory/nsSHistory.cpp
@@ +702,5 @@
>    /* GetTransactionAtIndex ensures aResult is valid and validates aIndex */
>    rv = GetTransactionAtIndex(aIndex, getter_AddRefs(txn));
>    if (NS_SUCCEEDED(rv) && txn) {
>      // Get the Entry from the transaction
> +    txn->GetSHEntry(aResult);

This method technically still returns a nsresult. It might be nicer to do it with a 

nsCOMPtr<nsISHEntry> entry = txn->GetSHEntry();
entry.forget(aResult);

to make it clear that we're infallible.

Comment 11

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

Comment on attachment 9006137 [details] [diff] [review]
Improve GetTransactionAtIndex() result checking

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

::: docshell/shistory/nsSHistory.cpp
@@ +612,5 @@
>  
> +  nsCOMPtr<nsISHTransaction> currentTxn;
> +  if (mIndex >= 0) {
> +    nsresult rv = GetTransactionAtIndex(mIndex, getter_AddRefs(currentTxn));
> +    NS_ENSURE_SUCCESS(rv, rv);

Previously this method would've not failed if mIndex >= Length(), right?

Comment 12

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

Comment on attachment 9006139 [details] [diff] [review]
Move the `persist` attribute from nsISHTransaction to nsISHEntry

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

::: docshell/shistory/nsISHEntry.idl
@@ +405,5 @@
> +     * When an entry is serving as a transaction within nsISHistory, this
> +     * property specifies if it should persist. If not it will be replaced by
> +     * new additions to the list.
> +     */
> +    [infallible] attribute boolean persist;

Perhaps we should assert that this value is only set & read from toplevel SHEntries?

Comment 13

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

Comment on attachment 9006141 [details] [diff] [review]
Fix up nsISHEntry.{index,getEntryAtIndex()}

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

::: toolkit/modules/sessionstore/SessionHistory.jsm
@@ +313,5 @@
>  
>      // Select the right history entry.
>      let index = tabData.index - 1;
>      if (index < history.count && history.index != index) {
> +      history.index = index;

A vast improvement across the board. Thanks!

Comment 14

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

Comment on attachment 9006142 [details] [diff] [review]
Remove all traces of the "transaction" terminology in SHistory

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

::: docshell/base/nsDocShell.cpp
@@ +2288,5 @@
>    // which session history entries are candidates for content viewer
>    // eviction.  We need to adjust by the number of entries that we
>    // just purged from history, so that we look at the right session history
>    // entries during eviction.
> +  if (aIndex == mPreviousEntryIndex) {  // njn: rename

nit: Can you add more detail to this todo?

Comment 15

[Nicholas Nethercote [inactive]]

> > +  if (aIndex == mPreviousEntryIndex) {  // njn: rename
> 
> nit: Can you add more detail to this todo?

Whoops, that's not supposed to be there :)  Thanks for spotting.

Comment 16

[Nicholas Nethercote [inactive]]

> Huh, I completely forgot I had landed [infallible] on interface types!

That was you? Cool. I think I was the one who landed the initial support for builtin types :)  I updated the XPIDL wiki page to say that it works with interface types.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Update comm-central for nsISHEntry.{index,getEntryAtIndex()} changes

Jorg, as usual, this compiles but I make no other promises :)

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9006400 [details] [diff] [review]
Update comm-central for nsISHEntry.{index,getEntryAtIndex()} changes

Thanks, but this is all suite/ code.

Comment 19

[Nicholas Nethercote [inactive]]

> > +    txn->GetSHEntry(aResult);
> 
> This method technically still returns a nsresult. It might be nicer to do it with a 
> 
> nsCOMPtr<nsISHEntry> entry = txn->GetSHEntry();
> entry.forget(aResult);
> 
> to make it clear that we're infallible.

It's a good suggestion, but GetSHEntry() is removed in a later patch so I won't bother changing it.

Comment 20

[Nicholas Nethercote [inactive]]

> Previously this method would've not failed if mIndex >= Length(), right?

Correct. I think it's reasonable for it to fail in that case, given that `mIndex >= Length()` should only occur if there is a bug within this code.

Comment 21

[Nicholas Nethercote [inactive]]

> Perhaps we should assert that this value is only set & read from toplevel SHEntries?

I tried doing this, by adding `!mParent` assertions in GetPersist() and SetPersist() but I'm getting tons of violations for GetPersist() calls that originate from JS code. Not sure why, perhaps I've misunderstood how parenting works. I will land the existing patches because I don't want that to block, but a follow-up is possible if you think it's important.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a18b3527ec
Remove `currentIndex` in nsSHistory::AddEntry(). r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/b737e972ee94
Make nsISHTransaction builtin and its attributes infallible. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe467bd12d8
Enforce non-nullness for nsISHTransaction.sHEntry. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a9359cbabc
Improve GetTransactionAtIndex() result checking. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/afb85694d4ff
Move the `persist` attribute from nsISHTransaction to nsISHEntry. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/107e7b610762
Remove nsISHTransaction. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f451bd99a45e
Fix up nsISHEntry.{index,getEntryAtIndex()}. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d48361106a
Remove all traces of the "transaction" terminology in SHistory. r=nika.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/89a18b3527ec
https://hg.mozilla.org/mozilla-central/rev/b737e972ee94
https://hg.mozilla.org/mozilla-central/rev/ebe467bd12d8
https://hg.mozilla.org/mozilla-central/rev/e0a9359cbabc
https://hg.mozilla.org/mozilla-central/rev/afb85694d4ff
https://hg.mozilla.org/mozilla-central/rev/107e7b610762
https://hg.mozilla.org/mozilla-central/rev/f451bd99a45e
https://hg.mozilla.org/mozilla-central/rev/00d48361106a

Comment 24

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9006400 [details] [diff] [review]
Update comm-central for nsISHEntry.{index,getEntryAtIndex()} changes

Thanks. Can't test it now because of other breakages but looks A++.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e7bcb952b29
Update comm-central for nsISHEntry.{index,getEntryAtIndex()} changes. r=frg