1276553 - Implement platform level GroupedSHistory.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Samael Wang [:freesamael] (away for now)]

To achieve the goal of bug 1276551, nsISHistory should support a partial mode, which simply implies "there are some entries not managed by current nsISHistory object", so that multiple nsISHistory in different root docshells (may even in different processes) can form a conceptual cross root-docshell session history.

nsISHistory should record the global length of the whole session history, and the offset of its indices in comparison to global indices.

Comment 1

[Samael Wang [:freesamael] (away for now)]

Attachment: Remove nsSHistoryEnumerator & make protected members private

Comment 2

[Samael Wang [:freesamael] (away for now)]

Attachment: Implement partial session history support

Comment 3

[Samael Wang [:freesamael] (away for now)]

Attachment: Update frameloader / browser for partial history

Comment 4

[Samael Wang [:freesamael] (away for now)]

Attachment: Add chrome mochitest test case

Comment 5

[Samael Wang [:freesamael] (away for now)]

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

Comment 6

[Samael Wang [:freesamael] (away for now)]

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

Comment 7

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Implement partial session history support

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Update frameloader / browser for partial history

Comment 9

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3: Add test case

Comment 10

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8760626 [details] [diff] [review]
Part 1: Implement partial session history support

Hi Olli,

This is a small piece of my attempt to implement cross root docshell session histories.

The basic idea is to record the offset and total history length to each nsSHistory of different root docshells. Those nsSHistory in combination compose a complete history list.

Could you help to review this patch? Or do you have any suggested reviewers to share your load?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

bz is on vacation and busy anyhow, sicking is on vacation too, so I don't really have other suggestions for reviewing session history patches.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8760626 [details] [diff] [review]
Part 1: Implement partial session history support

># HG changeset patch
># User Samael Wang <[e-mail]>
># Date 1465286056 -28800
>#      Tue Jun 07 15:54:16 2016 +0800
># Node ID b82b5829586cc2acfe252533e6e189072c70d88a
># Parent  1828937da9493b2cd54862b9c520b2ba5c7db92b
>Bug 1276553 - Part 1: Implement partial session history support.
>
>diff --git a/browser/components/sessionstore/content/content-sessionStore.js b/browser/components/sessionstore/content/content-sessionStore.js
>--- a/browser/components/sessionstore/content/content-sessionStore.js
>+++ b/browser/components/sessionstore/content/content-sessionStore.js
>@@ -340,16 +340,20 @@ var SessionHistoryListener = {
>     this.collect();
>     return true;
>   },
> 
>   OnHistoryReplaceEntry: function (index) {
>     this.collect();
>   },
> 
>+  OnRequestCrossBrowserNavigation: function(index) {
>+    sendAsyncMessage("SessionStore:navigation", {epoch: gCurrentEpoch, index});
>+  },
This feels wrong, to use sessionStore to implement part of the navigation.
Or do I misunderstand why this is added?

> nsSHistory::~nsSHistory()
> {
>@@ -414,40 +416,82 @@ nsSHistory::AddEntry(nsISHEntry* aSHEntr
>   // parent will properly set the parent child relationship
>   txn->SetPersist(aPersist);
>   NS_ENSURE_SUCCESS(txn->Create(aSHEntry, currentTxn), NS_ERROR_FAILURE);
> 
>   // A little tricky math here...  Basically when adding an object regardless of
>   // what the length was before, it should always be set back to the current and
>   // lop off the forward.
>   mLength = (++mIndex + 1);
>+  mExtraLength = 0;
So how do we clear the entries in other session histories after this entry?


> nsSHistory::GetCanGoBack(bool* aCanGoBack)
> {
>   NS_ENSURE_ARG_POINTER(aCanGoBack);
>+
>+  if (mGlobalIndexOffset) {
>+    *aCanGoBack = true;
>+    return NS_OK;
>+  }
>+
>+  NS_ENSURE_SUCCESS(canGoBackLocally(aCanGoBack), NS_ERROR_FAILURE);
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsSHistory::canGoBackLocally(bool* aCanGoBack)
>+{
Why you need this new method?


> nsSHistory::GetCanGoForward(bool* aCanGoForward)
> {
>   NS_ENSURE_ARG_POINTER(aCanGoForward);
>+
>+  if (mExtraLength) {
>+    *aCanGoForward = true;
>+    return NS_OK;
>+  }
>+
>+  NS_ENSURE_SUCCESS(canGoForwardLocally(aCanGoForward), NS_ERROR_FAILURE);
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsSHistory::canGoForwardLocally(bool* aCanGoForward)
Why you need this new method.
I don't think these methods make the code any easier to read.

> nsSHistory::LoadEntry(int32_t aIndex, long aLoadType, uint32_t aHistCmd)
> {
>+  if (aIndex < 0 || aIndex >= mLength) {
>+    if (aIndex + mGlobalIndexOffset < 0) {
>+      // the global index is negative
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    if (aIndex - mLength >= mExtraLength) {
>+      // the global index exceeds max possible value
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    // the global index is valid. trigger cross browser navigation.
>+    NOTIFY_LISTENERS(OnRequestCrossBrowserNavigation, (aIndex + mGlobalIndexOffset));
>+    return NS_OK;
>+  }
>+
Hmm, so aIndex refers to local index, and if it is negative or >= mLength, then crossbrowsernavigation may happen. 
But nsHistory::Go for example can't deal with that. It doesn't let aIndex to be < 0 or >= mLength

> 
>+  nsresult canGoBackLocally(bool* aCanGoBack);
>+  nsresult canGoForwardLocally(bool* aCanGoForward);
Methods in C++ should be in form CamelCase, so, capital first letter.


>+  // number of entries before this session history object
>+  int32_t mGlobalIndexOffset;
>+  // number of entries after this session history object
>+  int32_t mExtraLength;
Hmm, a bit odd name.
Perhaps, mEntriesInFollowingPartialHistories ? A bit long but would be easier to understand.


Mostly just small nits, though nsHistory::Go is a larger thing.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8760627 [details] [diff] [review]
Part 2: Update frameloader / browser for partial history

>+nsFrameLoader::SetGroupedHistoryInfo(int32_t aOffset, int32_t aGlobalLength)
>+{
>+  if (IsRemoteFrame()) {
>+    if (!mRemoteBrowser) {
>+      NS_WARNING("Missing remote browser.");
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    Unused << mRemoteBrowser->SendSetGroupedHistoryInfo(aOffset, aGlobalLength);
>+  } else {
>+    if (!mDocShell) {
>+      NS_WARNING("Missing docshell.");
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>+    nsCOMPtr<nsISHistory> shistory;
>+    webNav->GetSessionHistory(getter_AddRefs(shistory));
>+    if (!shistory) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    nsresult rv = shistory->SetGroupedHistoryInfo(aOffset, aGlobalLength);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
it is unclear to me whether we ever want to support non-remote case here.
May be do, and if it is easy to do, fine.


>       <method name="makePrerenderedBrowserActive">
>+        <parameter name="aHistoryOffset"/>
>+        <body>
>+        <![CDATA[
>+          this.removeAttribute("prerendered");
>+          let offset = (aHistoryOffset != undefined) ? aHistoryOffset : 0;
>+          let frameLoader = this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader;
>+          if (frameLoader) {
>+            frameLoader.makePrerenderedLoaderActive();
>+            // when activating a prerendered page,
>+            // the global length is always offset + 1
>+            frameLoader.setGroupedHistoryInfo(offset, offset + 1);
So, hmm, how does this work... shouldn't we update the history right before making the prerendered loader active. Otherwise the active page might already do stuff with its session history.


>+      <method name="setGroupedHistoryInfo">
>+        <parameter name="aOffset"/>
>+        <parameter name="aGlobalLength"/>
>         <body>
>         <![CDATA[
>           let frameLoader = this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader;
>           if (frameLoader) {
>-            frameLoader.makePrerenderedLoaderActive();
>+            frameLoader.setGroupedHistoryInfo(aOffset, aGlobalLength);
Somewhat similar question here. We can't really have the frameloader's docshell or relevant TabParent/TabChild active when updating grouped history, otherwise the session history might have used invalid 
data. So, can we somehow check here that frameloader isn't active or something?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8760628 [details] [diff] [review]
Part 3: Add test case

rs+, though I wonder if this need some changes if we require setGroupedHistoryInfo to be set early enough.

s/goForard/goForward/

We should get some tests also for history.go() usage, at least when we actually swap frameloaders.

Comment 15

[Samael Wang [:freesamael] (away for now)]

My plan was to deal with nsHistory separately in bug 1276552, sorry I forgot to mention that.

The approach of my cross-docshell history design was actually built on top of SessionStore. The idea is that we need complete history list to be in chrome process, and SessionStore already has those information, so in my prototype I built GroupedSessionHistory [1] by reusing SessionStore. 

Another approach I can think of is to collect and store history in frameloader on each swapping, but I'm not sure it's a better design to have 2 different places collecting session history information in chrome process. Is there any other approach you suggest?

[1] https://github.com/freesamael/gecko-dev/commit/8c8d47cbc1f1035b99adcd664d4cff3f537a5a0c

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Samael Wang [:freesamael][:sawang] from comment #15)
> My plan was to deal with nsHistory separately in bug 1276552, sorry I forgot
> to mention that.
I see. totally fine. thanks.

> 
> The approach of my cross-docshell history design was actually built on top
> of SessionStore. The idea is that we need complete history list to be in
> chrome process, and SessionStore already has those information, so in my
> prototype I built GroupedSessionHistory [1] by reusing SessionStore. 
Oh, why do we need complete history in chrome? Don't we just need the number of indexes in all the partial histories?

> Another approach I can think of is to collect and store history in
> frameloader on each swapping, but I'm not sure it's a better design to have
> 2 different places collecting session history information in chrome process.
> Is there any other approach you suggest?
Well, I thought we'd need just the length of all the partial histories in parent and then
send that offset and global length to TabChild just before we're about to activate it.
Since that is quite simple list, it could be kept in nsFrameLoader or whoever will deal with the swapping.

But if session store approach works too, perhaps that is fine. The patches in this bug currently just
use SessionHistoryListener's OnRequestCrossBrowserNavigation and nothing else, so I think I'm missing something here still. How are you planning to use sessionstore in parent?

Comment 17

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #16)
> (In reply to Samael Wang [:freesamael][:sawang] from comment #15)
> > The approach of my cross-docshell history design was actually built on top
> > of SessionStore. The idea is that we need complete history list to be in
> > chrome process, and SessionStore already has those information, so in my
> > prototype I built GroupedSessionHistory [1] by reusing SessionStore. 
> Oh, why do we need complete history in chrome? Don't we just need the number
> of indexes in all the partial histories?

Oh I was also considering session restore use cases. When firefox crashes and user launches the browser again, SessionStore needs to somehow know there exists a session composed of multiple session history records, and ideally restoring the complete history into a single tab as we don't really need multiple tabs in this case. 

Either we can store the relationship between tabs in the SessionStore and compose the complete history list on restoring, or we can store a complete history list at first place. I happen to use the later approach for both navigation and session restore cases.

> > Another approach I can think of is to collect and store history in
> > frameloader on each swapping, but I'm not sure it's a better design to have
> > 2 different places collecting session history information in chrome process.
> > Is there any other approach you suggest?
> Well, I thought we'd need just the length of all the partial histories in
> parent and then
> send that offset and global length to TabChild just before we're about to
> activate it.
> Since that is quite simple list, it could be kept in nsFrameLoader or
> whoever will deal with the swapping.
> 
> But if session store approach works too, perhaps that is fine. The patches
> in this bug currently just
> use SessionHistoryListener's OnRequestCrossBrowserNavigation and nothing
> else, so I think I'm missing something here still. How are you planning to
> use sessionstore in parent?

In content-sessionStore.js it sends a message "SessionStore:navigation" with a global index to the chrome process when OnRequestCrossBrowserNavigation() is called [1].

SessionStore.jsm in chrome process will handle this message and invoke tabbrowser.gotoIndex(). tabbrowser will then use information stored in GroupedSessionHistory.jsm to do proper navigation. This part is not well tested yet and is still a bit buggy. It will be covered in bug 1276551.

I do feel hesitate when making those design decisions. Sometimes I don't really know which approach is better. I'll be happy to hear more feedback.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8760626&action=diff#a/browser/components/sessionstore/content/content-sessionStore.js_sec2

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Samael Wang [:freesamael][:sawang] from comment #17)
> Oh I was also considering session restore use cases. When firefox crashes
> and user launches the browser again, SessionStore needs to somehow know
> there exists a session composed of multiple session history records, and
> ideally restoring the complete history into a single tab as we don't really
> need multiple tabs in this case. 
I see



> I do feel hesitate when making those design decisions. Sometimes I don't
> really know which approach is better. I'll be happy to hear more feedback.
Sounds like the SessionStore approach might be quite ok. In general I'm not too happy to couple session history handling to session store, but if that eases implementation, fine.
(SessionStore is considered a Firefox feature, session history is part of Gecko. Different owners of the code, different reviewers usually.)

Comment 19

[Samael Wang [:freesamael] (away for now)]

Attachment: (WIP) Implement platform level grouped session history

Comment 20

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8787594 [details] [diff] [review]
(WIP) Implement platform level grouped session history

Hi Olli,

I made a WIP patch to implement grouped session history at frameloader level. 

The major issue at this moment is the actual swapping part. If I make swapping 
at frameloader directly, nsXULElement::mSlots won't be updated, so as some 
<xul:browser> attributes. I'm thinking whether I should try to make swapping 
happen at nsIFrameLoaderOwner level or <xul:browser> script.

Other minor issues include:

1. I'm a bit hesitate at interface design, mainly caused by the fact that 
gecko doesn't have nsIWebNavigation implementation for e10s. It was done by 
firefox in RemoteWebNavigation.js. I need something like get history count / 
goto index and I'm not sure where the right interface is to put these functions.

2. I'm not sure whether session history is always available in TabChild. 
Does <iframe mozbrowser> uses TabParent / TabChild? I'm not sure if my patch 
will break <iframe mozbrowser>.

3. I haven't figure out how to safely close a tab at frameloader level. 
Is TabParent::Destroy() the right way to do?

4. Should I use native pointers or nsWeakPtr when I don't want to use strong 
reference?

5. Do you think we should support non-e10s?

I'm trying to rebuild prerender prototype on top of the WIP, and figure out 
these issues. In the mean time I'll be very happy to hear your feedback.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(1) at least the name nsISHistoryAttributesCache is a but odd, or I don't quite understand it.
Is nsISHistoryAttributesCache something like nsIGroupedSHistorySlice, representing a part of the grouped SHistory

(2) for remote mozbrowser TabParent/Child is being used.

(3) yes, but also check what other stuff nsFrameLoader::Destroy() ends up doing normally.

(4) if possible, use nsWeakPtr or similar, or if raw pointers, then clearly document what guarantees the pointers never point to a deleted object.

(5) I think this stuff won't make much sense in non-e10. Though, hmm, what about pages loaded in parent process only. Say about:preferences. Need to figure out something for that.

But I'll look at the patch some more

Comment 22

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #21)
> (1) at least the name nsISHistoryAttributesCache is a but odd, or I don't
> quite understand it.
> Is nsISHistoryAttributesCache something like nsIGroupedSHistorySlice,
> representing a part of the grouped SHistory
> 

Naming has always been difficult...

The story was like this: 

I was looking for a way to get content process's shistory count and offset from parent process.
Firstly I thought about making TabParent implements nsISHistory, but that sounds terrible and 
I'll leave a lot of NS_ERROR_NOT_IMPLEMENTED functions. Then I thought about SHistoryParent or
nsIRemoteSHistory, and also tried nsIPartialSHistory. I end up with nsISHistoryAttributesCache 
simply meaning it's used to cache attributes of nsISHistory. Still looking for a better name though.

GroupedSHistory actually holds a list of nsFrameLoader at this moment.

I felt that in an ideal world GroupdSHistory would operate on some kind of e10s version of 
nsIWebNavgiation & nsISHistory but they don't exist, so I was looking for something to represent 
a cached state of nsISHistory in content process and added GotoIndex to PBrowser & frameloader.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(Sorry I'm late with feedback. Ping me if I don't give feedback today.)

Comment 24

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #23)
> (Sorry I'm late with feedback. Ping me if I don't give feedback today.)

It seems the frameloader swapping part really need <tabbrowser> to be involved. 
I couldn't decide how this can be done properly. I need either the frameloader to notify <tabbrowser> 
that 2 tabs have been swapped, or let <tabbrowser> to initiate swapping at first place.

Could you give me some suggestions on this when you look at the patch?

> +NS_IMETHODIMP
> +nsFrameLoader::RequestCrossLoaderNavigation(int32_t aGlobalIndex)
> +{
> +  NS_ENSURE_TRUE(mGroupedSessionHistory, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIFrameLoader> targetLoader;
> +  int32_t targetIndex;
> +  nsresult rv = mGroupedSessionHistory->FindLoaderAndIndexAt(
> +                  aGlobalIndex, getter_AddRefs(targetLoader), &targetIndex);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(targetLoader, NS_ERROR_UNEXPECTED);
> +
> +  RefPtr<nsFrameLoader> otherLoader = static_cast<nsFrameLoader*>(targetLoader.get());
> +  if (targetLoader != this) {
> +    // FIXME: This won't work either.
> +    // See nsFrameLoader::AppendPartialSessionHistoryAndSwapTo()
> +    RefPtr<nsFrameLoader> ourLoader(this);
> +    SwapWithOtherLoader(otherLoader, ourLoader, otherLoader);
> +    mGroupedSessionHistory.swap(otherLoader->mGroupedSessionHistory);
> +  }

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8787594 [details] [diff] [review]
(WIP) Implement platform level grouped session history

>+  /**
>+   * Called when the session history object needs to initiate
>+   * cross-root-docshell navigation. It implies the session history object is a
>+   * partial history, and the complete history is composed of multiple session
>+   * history objects across root docshells.
>+   *
>+   * TODO: It's not quite a "notification" but rather a "request". Should we
>+   * create a new interface for this method separately?
Yeah, I think so. Even though we do have already quite many interfaces, this is so different from
other methods in this inteface, I'd add a new one.
And SHistory should allow only one this new kind of callback.



nsISHistoryAttributesCache is still a bit odd name.
Maybe nsIPartialSHistoryStatus or some such? or nsICachedPartialSHistoryStatus?


>+nsFrameLoader::AppendPartialSessionHistoryAndSwapTo(nsIFrameLoader* aOther)
>+{
>+  NS_ENSURE_ARG_POINTER(aOther);
>+
>+  if (!mGroupedSessionHistory) {
>+    mGroupedSessionHistory = new GroupedSHistory();
>+    mGroupedSessionHistory->AppendLoader(this);
>+  }
>+
>+  nsresult rv = mGroupedSessionHistory->AppendLoader(aOther);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // FIXME: This won't work. nsXULElement::mSlots are not swapped.
>+  // Also need to check what tabbrowser._swapBrowserDocShells and
>+  // browser.swapDocShells do. Maybe we should eventually ask swapping
>+  // at <xul:tabbrowser> level.
Add a getter to XUL element to access its slots->RefPtr<nsFrameLoader> variable as reference and the use that for swapping?

>+  RefPtr<nsFrameLoader> otherLoader = static_cast<nsFrameLoader*>(targetLoader.get());
>+  if (targetLoader != this) {
>+    // FIXME: This won't work either.
>+    // See nsFrameLoader::AppendPartialSessionHistoryAndSwapTo()
>+    RefPtr<nsFrameLoader> ourLoader(this);
>+    SwapWithOtherLoader(otherLoader, ourLoader, otherLoader);
and same here.

Comment 26

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

I think I would want to use this in non-prerendering cases too. This would greatly simplify the case of cross process navigations, either from one process into a new process (for bug 1277066), or from content into chrome and vice-versa.

How difficult would it be to support having a piece of session history be in the parent process instead? (as in, would this be able to handle the case of navigating from http://foo.com to about:config, and hitting the back button). It could be a lot nicer than our current system for changing processes to load a URL, which uses sessionstore to transplant the history entries into a fresh browsing context.

Comment 28

[Samael Wang [:freesamael] (away for now)]

(In reply to Michael Layzell [:mystor] from comment #26)
> I think I would want to use this in non-prerendering cases too. This would
> greatly simplify the case of cross process navigations, either from one
> process into a new process (for bug 1277066),

We do want to support swapping between 2 content processes since our long term goal is out-of-process prerendering. The implementation here should prepare for this case.

> or from content into chrome and vice-versa.
> How difficult would it be to support having a piece of session history be in
> the parent process instead? (as in, would this be able to handle the case of
> navigating from http://foo.com to about:config, and hitting the back
> button). 

I wasn't sure if we want to support swapping between chrome / content processes, but you remind me that we've once discussed the possibility of triggering prerender in about:newtab or things like activity stream, which may require chrome / content swapping.

The problem is that nsFrameLoader::SwapWithOtherLoader expects either both loaders live in chrome process or both live in content processes. Implementing swapping across chrome / content process could be considerably complex.

Session history itself is relatively easier since we can access chrome docshell's nsSHistory directly.

> It could be a lot nicer than our current system for changing
> processes to load a URL, which uses sessionstore to transplant the history
> entries into a fresh browsing context.

There was an attempt to implement cross-process session history in this way (serializing and then restoring session store) in bug 1209662. The reason we didn't adopt it for prerendering is because we want to keep bfcache works. The proposed solution for prerendering is that we'll keep a list of hidden tabs (literally <xul:tab hidden="true">) within the visible active tab, build session history across these tabs, and swap frameloaders of these tabs on history back / forward.

The drawback is that we'll keep all of these tabs open in background. There're just hidden, not closed. This should indicate more memory consumption, although I have no idea how much it will be. Does this sound reasonable to your use case?

One major issue remaining is that the swapped out docshells are still active, which means web content scripts are still running. My plan is to make it behave as in bfcache in a follow up bug (and hopefully implement it in Q4). Either I should create a pseudo SHEntry and transfer the active content viewer to the SHEntry on swapping out, or somehow make the whole docshell freeze... not sure, I haven't dig into it yet.

Comment 29

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

I assume that these hidden tabs are cleaned up eventually, like bfcache entries. If that is the case, then that sounds like exactly like what I would ideally want for my work.

Comment 30

[Samael Wang [:freesamael] (away for now)]

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

Comment 31

[Samael Wang [:freesamael] (away for now)]

Attachment: Implement platform level grouped session history

finally got something

Comment 32

[Samael Wang [:freesamael] (away for now)]

Attachment: Implement platform level grouped session history

...yet forgot to remove one file

Comment 33

[Samael Wang [:freesamael] (away for now)]

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

Comment 34

[Samael Wang [:freesamael] (away for now)]

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

Comment 35

[Samael Wang [:freesamael] (away for now)]

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

Comment 36

[Samael Wang [:freesamael] (away for now)]

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

Comment 37

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 1: Make nsISHistory support partial mode.

Make nsISHistory support partial mode and create interfaces which
DOM level grouped session history should implement.

Review commit: https://reviewboard.mozilla.org/r/85816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85816/

Comment 38

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 2: Add groupedSHistory related functions to nsIBrowser.

Move nsIBrowser to dom/interfaces and add functions GroupedSHistory needs.

nsIBrowser looks not strictly related to IPC as <xul:browser> implements
nsIBrowser whereas <xul:remote-browser> implements nsIRemoteBrowser, and
even nsIRemoteBrowser lives in dom/interfaces.

Review commit: https://reviewboard.mozilla.org/r/85818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85818/

Comment 39

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review commit: https://reviewboard.mozilla.org/r/85820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85820/

Comment 40

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 4: Add test case.

Review commit: https://reviewboard.mozilla.org/r/85822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85822/

Comment 41

[Samael Wang [:freesamael] (away for now)]

The latest try looks better now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e683a2df7eae60119eb17a6be3f8c49f61b387

I'm expecting more review iterations on this bug so let's try if mozreview helps. Follow-up this bug, we'll need:

1. Find a solution for mixed chrome / content partial histories. The current implementation breaks when user navigates back and forth to about:config and then a normal e10s page.

2. Active document in a swapped out partial history is still active. We need to suspend the document as how bfcache works.

Comment 42

[Samael Wang [:freesamael] (away for now)]

Hi Michael,

MozReview does not support feedback?, but since we're aligning our implementation, I'd be happy to hear your feedback on the purposed patches.

Comment 43

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

(In reply to Samael Wang [:freesamael][:sawang] from comment #42)
> Hi Michael,
> 
> MozReview does not support feedback?, but since we're aligning our
> implementation, I'd be happy to hear your feedback on the purposed patches.

I didn't read the APIs too deeply, but from what I've seen this looks pretty awesome. As far as I can tell when adding a new segment into the history, I simply need to create the xul:browser element, and ask it to swap with appendPartialSessionHistoryAndSwap, and then the partial session history logic will handle switching back when performing history navigations? That sounds pretty straightforward and good :).

Right now the updateBrowserRemoteness logic (http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1653-1759) does a bunch of work to hook up listeners and such again to the new browser when it is reattached. does this patch also handle keeping that stuff connected correctly?

Comment 44

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/1-2/

Comment 45

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 1: Make nsISHistory support partial mode.

Make nsISHistory support partial mode and create interfaces which
DOM level grouped session history should implement.

Review commit: https://reviewboard.mozilla.org/r/86486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86486/

Comment 46

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

Move nsIBrowser to dom/interfaces and add functions GroupedSHistory needs.

nsIBrowser looks not strictly related to IPC as <xul:browser> implements
nsIBrowser whereas <xul:remote-browser> implements nsIRemoteBrowser, and
even nsIRemoteBrowser lives in dom/interfaces.

Review commit: https://reviewboard.mozilla.org/r/86488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86488/

Comment 47

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 5: Add test case.

Review commit: https://reviewboard.mozilla.org/r/86490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86490/

Comment 48

[Samael Wang [:freesamael] (away for now)]

Sorry for mis-operation. I was just trying to update one of them.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801894 [details]
Bug 1276553 - Part 1: Make nsISHistory support partial mode.

https://reviewboard.mozilla.org/r/86486/#review85324

::: docshell/shistory/nsIGroupedSHistory.idl:13
(Diff revision 1)
> +interface nsIFrameLoader;
> +interface nsIPartialSHistoryStatus;
> +
> +/**
> + * nsIGroupedSHistory represent a combined session history across multiple
> + * root docshells (usually browser tabs). The participated nsISHistory can

Should it be 'participating', not 'participated'

::: docshell/shistory/nsIPartialSHistoryStatus.idl:17
(Diff revision 1)
> + * nsIPartialSHistoryStatus represents a part of nsIGroupedSHistory. It keeps
> + * status associated to a "partial" nsISHistory in either local or remote
> + * process.
> + */
> +[scriptable, builtinclass, uuid(5cd75e28-838c-4a0a-972e-6005f736ef7a)]
> +interface nsIPartialSHistoryStatus : nsISupports

Would it be ok to call this nsIPartialSHistory .
*Status makes it sound, at least to me, like some simple object representing just some status, but this is actually more complicated.

::: docshell/shistory/nsISHistoryListener.idl:107
(Diff revision 1)
> +   /**
> +    * Called when nsISHistory::count has been updated. Unlike OnHistoryNewEntry
> +    * and OnHistoryPurge which happen before the modifications are actually done
> +    * and maybe cancellable, this function is called after these modifications.
> +    */
> +   void OnLengthChange(in long aCount);

And this isn't about global grouped session history, but this local (old style) session history, right?

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

https://reviewboard.mozilla.org/r/86488/#review85336

::: dom/interfaces/base/nsIBrowser.idl:9
(Diff revision 1)
> +#include "nsISupports.idl"
> +
> +interface nsIDOMElement;
> +
> +[scriptable, uuid(14e5a0cb-e223-4202-95e8-fe53275193ea)]
> +interface nsIBrowser : nsISupports

You should hg mv this file, not delete the old and create a new one.

::: toolkit/content/widgets/browser.xml:1195
(Diff revision 1)
>          </body>
>        </method>
>  
> +      <method name="closeWindow">
> +        <body>
> +        <![CDATA[

Some toolkit or browser peer should also review browser.xml changes.

::: toolkit/content/widgets/browser.xml:1208
(Diff revision 1)
> +              return;
> +            }
> +          }
> +
> +          // If we're not attached to a tabbrowser, just destroy.
> +          this.destroy();

This looks wrong. Just destroying doesn't seem to remove the browser element or anything

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

https://reviewboard.mozilla.org/r/86488/#review85338

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

https://reviewboard.mozilla.org/r/85820/#review85342

::: dom/base/GroupedSHistory.h:34
(Diff revision 2)
> +
> +  /**
> +   * Remove all partial histories and close tabs after the given index (of
> +   * mPartialHistories, not the index of session history entries).
> +   */
> +  void PurgePartialHistories(int32_t lastIndexToKeep);

Nit, aLastIndexToKeep

::: dom/base/GroupedSHistory.h:36
(Diff revision 2)
> +   * Remove all partial histories and close tabs after the given index (of
> +   * mPartialHistories, not the index of session history entries).
> +   */
> +  void PurgePartialHistories(int32_t lastIndexToKeep);
> +
> +  int32_t mCount;

What is this mCount about? The number of entries in all the partial histories? Please add a comment.

::: dom/base/GroupedSHistory.h:38
(Diff revision 2)
> +   */
> +  void PurgePartialHistories(int32_t lastIndexToKeep);
> +
> +  int32_t mCount;
> +  int32_t mIndexOfActivePartialHistory;
> +  nsTArray<nsWeakPtr> mPartialHistories;

What does this array contain? Please add a comment

::: dom/base/GroupedSHistory.cpp:125
(Diff revision 2)
> +  NS_WARNING("Out of index request!");
> +  return NS_ERROR_FAILURE;
> +}
> +
> +void
> +GroupedSHistory::PurgePartialHistories(int32_t lastIndexToKeep)

aLastIndexToKeep

But since this is about index in partial history arrays, could you perhaps rename the argument.
aLastPartialIndexToKeep ? Same also in the .h file.

::: dom/base/PartialSHistoryStatus.h:22
(Diff revision 2)
> +#include "TabParent.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class PartialSHistoryStatus final : public nsIPartialSHistoryStatus,

As I mentioned in the other patch, I think 
PartialSHistoryStatus could be renamed.
Perhaps just PartialSHistory

::: dom/base/PartialSHistoryStatus.h:46
(Diff revision 2)
> +  already_AddRefed<nsISHistory> GetSessionHistory();
> +  already_AddRefed<TabParent> GetTabParent();
> +
> +  int32_t mCount;
> +  int32_t mGlobalIndexOffset;
> +  nsWeakPtr mGroupedSHistory;

Why we need GroupedSHistory here when OwnerFrameLoader has a pointer to it too?

::: dom/base/PartialSHistoryStatus.h:48
(Diff revision 2)
> +
> +  int32_t mCount;
> +  int32_t mGlobalIndexOffset;
> +  nsWeakPtr mGroupedSHistory;
> +
> +  // Since the owner of PartialSHistoryStatus _is_ this frame loader, there

What clears mOwnerFrameLoader when the frameloader is deleted?
PartialSHistoryStatus is refcounted so nothing guarantees that it is deleted before nsFrameloader.

So, nsFrameLoader must explicitly call some Disconnect() or such method on PartialSHistory to clear mOwnerFrameLoader pointer.

And I wouldn't mind adding some nullchecks for mOwnerFrameLoader then.

::: dom/base/nsFrameLoader.cpp:443
(Diff revision 2)
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsFrameLoader::RequestCrossLoaderNavigation(int32_t aGlobalIndex)

CrossLoader? What does that mean.
Why not RequestHistoryNavigation ?

::: dom/base/nsFrameLoader.cpp:3331
(Diff revision 2)
>  
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsFrameLoader::RequestWindowClose()

Could we call this something else than RequestWindowClose()?

CloseFrameLoader() ?

::: dom/ipc/PBrowser.ipdl:894
(Diff revision 2)
> +     *                          history before this session history object.
> +     */
> +    async NotifyAttachGroupedSessionHistory(int32_t aOffset);
> +
> +    /**
> +     * Notify that the session history asssociates to this PBrowser has became

associates

has become

::: dom/ipc/PBrowser.ipdl:899
(Diff revision 2)
> +     * Notify that the session history asssociates to this PBrowser has became
> +     * the active history in the grouped session history.
> +     *
> +     * @param aGlobalLength      The up-to-date number of entries in the grouped
> +     *                           session history.
> +     * @param aTargetIndex       The target index to navigate to.

Is this index a global index or per partial shistory? Please improve the comment and perhaps change the name of the argument.

::: dom/ipc/PBrowser.ipdl:904
(Diff revision 2)
> +     * @param aTargetIndex       The target index to navigate to.
> +     */
> +    async NotifyPartialSessionHistoryActive(int32_t aGlobalLength, int32_t aTargetIndex);
> +
> +    /**
> +     * Notify that the session history asssociates to this PBrowser has became

has become

::: dom/ipc/TabChild.h:186
(Diff revision 2)
> +  NS_DECL_NSISHISTORYLISTENER
> +  NS_DECL_NSIPARTIALSHISTORYLISTENER
> +
> +private:
> +  ~TabChildSHistoryListener() {}
> +  TabChild* mTabChild;

What guarantees mTabChild never ever points to a deleted object?

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

https://reviewboard.mozilla.org/r/86490/#review85636

Mostly rs+

Comment 54

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

https://reviewboard.mozilla.org/r/85820/#review85342

> CrossLoader? What does that mean.
> Why not RequestHistoryNavigation ?

I was trying to indicate it will navigate to another frameloader. What about RequestGroupedHistoryNavigation? Since the function is strictly for groupedSHistory only.

> Could we call this something else than RequestWindowClose()?
> 
> CloseFrameLoader() ?

Wouldn't it sound too much like nsFrameLoader::Destory()? What about CloseOwnerElement()?

Comment 55

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801894 [details]
Bug 1276553 - Part 1: Make nsISHistory support partial mode.

https://reviewboard.mozilla.org/r/86486/#review85324

> And this isn't about global grouped session history, but this local (old style) session history, right?

Yes. GroupedSHistory needs to know when number of entries of nsISHistory changes. I was trying to use OnHistoryNewEntry and OnHistoryPurge at first but later I found they're not suitable since since the length is not updated yet.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

RequestGroupedHistoryNavigation... ok, fine.


closing an element doesn't quite have meaning. One may remove element from DOM or so, but close, not really.
RequestFrameLoaderClose()? (since it is IMO implementation detail that element is removed from DOM).

Comment 57

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #56)
> closing an element doesn't quite have meaning. One may remove element from
> DOM or so, but close, not really.
> RequestFrameLoaderClose()? (since it is IMO implementation detail that
> element is removed from DOM).

Do you think I should also change nsIBrowser.closeWindow() to a more specific name, like removeTab()?

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Hmm, nsIBrowser might have .close(), or closeBrowser() ?

Comment 59

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Add swapBrowsers() for frameloader or other platform components to swap frameloaders
and <xul:browser> listeners.

Add closeBrowser() for chrome process top-level frameloader to correctly remove /
close a tab.

Review commit: https://reviewboard.mozilla.org/r/87194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87194/

Comment 60

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801894 [details]
Bug 1276553 - Part 1: Make nsISHistory support partial mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86486/diff/1-2/

Comment 61

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86488/diff/1-2/

Comment 62

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/2-3/

Comment 63

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/1-2/

Comment 64

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

https://reviewboard.mozilla.org/r/86488/#review85336

> Some toolkit or browser peer should also review browser.xml changes.

Seperated to another commit for that.

> This looks wrong. Just destroying doesn't seem to remove the browser element or anything

It looks I shouldn't do anything in this case since I couldn't determine what to do if it's not attached to a tabbrowser.

Comment 65

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87194/diff/1-2/

Comment 66

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/3-4/

Comment 67

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/2-3/

Comment 68

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

https://reviewboard.mozilla.org/r/85820/#review85342

> What clears mOwnerFrameLoader when the frameloader is deleted?
> PartialSHistoryStatus is refcounted so nothing guarantees that it is deleted before nsFrameloader.
> 
> So, nsFrameLoader must explicitly call some Disconnect() or such method on PartialSHistory to clear mOwnerFrameLoader pointer.
> 
> And I wouldn't mind adding some nullchecks for mOwnerFrameLoader then.

I was confused about the ref counting / garbage collection stuff. Had a short discussion with Ehsan to sort it out, and now I'm trying to use cycle collection instead. I add better document at GroupedSHistory.h describing the relationship between frameloader / partial history and grouped history, and try to make them all participate cycle collection.

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

closeBrowser should probably throw an exception when it couldn't call removeTab?

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

https://reviewboard.mozilla.org/r/86488/#review86636

ok, so part 2 is now very different than what it was in revision 1.
So we need to do the move? I guess the new place at least isn't any worse than the old one, so fine.

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

https://reviewboard.mozilla.org/r/87194/#review86640

::: toolkit/content/widgets/browser.xml:1193
(Diff revision 2)
>            }
>          ]]>
>          </body>
>        </method>
>  
> +      <method name="closeBrowser">

I think should, for now at least, throw some exception when <browser> isn't in a tabbrowser.
That hopefully prevents one to use this method in wrong context.

Comment 72

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

https://reviewboard.mozilla.org/r/85820/#review86668

::: dom/base/GroupedSHistory.cpp:139
(Diff revision 4)
> +  NS_WARNING("Out of index request!");
> +  return NS_ERROR_FAILURE;
> +}
> +
> +void
> +GroupedSHistory::PurgePartialHistories(int32_t aLastPartialIndexToKeep)

I think this should take uint32_t, not int32_t

::: dom/base/GroupedSHistory.cpp:151
(Diff revision 4)
> +  }
> +
> +  size_t entriesToRemove = length - firstIndexToRemove;
> +
> +  // Close tabs.
> +  for (size_t i = 0; i < entriesToRemove; i++) {

Wouldn't this be a bit simpler if you did
for (size_t i = length - 1; i > aLastPartialIndexToKeep; --i) {
  nsCOMPtr<nsIPartialSHistory> partialHistory = mPartialHistories[i];

...

}

::: dom/base/PartialSHistory.h:25
(Diff revision 4)
> +namespace dom {
> +
> +class PartialSHistory final : public nsIPartialSHistory,
> +                              public nsISHistoryListener,
> +                              public nsIPartialSHistoryListener,
> +                              public nsSupportsWeakReference

Aha, nsSupportsWeakReference is still needed because nsSHistory has a WeakRef to PartialHistory.

::: dom/base/nsFrameLoader.cpp:404
(Diff revision 4)
> +  groupedHistory.forget(aResult);
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsFrameLoader::AppendPartialSessionHistoryAndSwap(nsIFrameLoader* aOther)

To self document this method, I think we should return an error value early if aOther has mGroupedSessionHistory is set. And also add MOZ_ASSERT for that case to crash in debug builds.

::: dom/base/nsFrameLoader.cpp:448
(Diff revision 4)
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsFrameLoader::RequestGroupedHistoryNavigation(int32_t aGlobalIndex)

Shouldn't this take uint32_t. So, unsigned long in .idl

::: dom/base/nsFrameLoader.cpp:2206
(Diff revision 4)
>  
>      nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>      webNav->SetSessionHistory(sessionHistory);
> +
> +
> +    mPartialSessionHistory = new PartialSHistory(this);

I think we need some pref here for now to ensure we don't create useless partial session histories.
Tests could enable that pref, but by default it would be false until we have some use for this code 
(like prerendering).

::: dom/base/nsIFrameLoader.idl:85
(Diff revision 4)
> +
> +  /**
> +   * If grouped session history is applied, use this function to navigate to
> +   * an entry of session history object of another frameloader.
> +   */
> +  void requestGroupedHistoryNavigation(in long aGlobalIndex);

unsigned long?

::: dom/ipc/PBrowser.ipdl:608
(Diff revision 4)
> +     * Notify parent that one or more entries have been added / removed from
> +     * the child session history.
> +     *
> +     * @param aCount the updated number of entries in child session history
> +     */
> +    async NotifySessionHistoryChange(int32_t aCount);

uint32_t?

::: dom/ipc/PBrowser.ipdl:617
(Diff revision 4)
> +     * is used to notify parent that it needs to navigate to an entry out of
> +     * local index of the child.
> +     *
> +     * @param aGlobalIndex The global index of history entry to navigate to.
> +     */
> +    async RequestCrossBrowserNavigation(int32_t aGlobalIndex);

uint32_t ?

::: dom/ipc/PBrowser.ipdl:891
(Diff revision 4)
> +     * attached to a grouped session history.
> +     *
> +     * @param aOffset           The number of entries in the grouped session
> +     *                          history before this session history object.
> +     */
> +    async NotifyAttachGroupedSessionHistory(int32_t aOffset);

uint32_t

::: dom/ipc/PBrowser.ipdl:894
(Diff revision 4)
> +     *                          history before this session history object.
> +     */
> +    async NotifyAttachGroupedSessionHistory(int32_t aOffset);
> +
> +    /**
> +     * Notify that the session history associates to this PBrowser has become

Hmm, should it be 'associated'

::: dom/ipc/PBrowser.ipdl:901
(Diff revision 4)
> +     *
> +     * @param aGlobalLength      The up-to-date number of entries in the grouped
> +     *                           session history.
> +     * @param aTargetLocalIndex  The target local index to navigate to.
> +     */
> +    async NotifyPartialSessionHistoryActive(int32_t aGlobalLength,

uint32_t for both params?

::: dom/ipc/TabChild.cpp:838
(Diff revision 4)
>        });
>    mAPZEventState = new APZEventState(mPuppetWidget, Move(callback));
>  
>    mIPCOpen = true;
>  
> +  // Set session history listener.

I think also here we need some pref check to ensure we don't use this stuff before actually needed.

::: dom/ipc/TabParent.cpp:3476
(Diff revision 4)
> +  }
> +
> +  nsCOMPtr<nsIPartialSHistory> partialHistory;
> +  frameLoader->GetPartialSessionHistory(getter_AddRefs(partialHistory));
> +  if (!partialHistory) {
> +    return false;

really, return false? I don't think we want to crash child process in such case. Return true

Comment 73

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801894 [details]
Bug 1276553 - Part 1: Make nsISHistory support partial mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86486/diff/2-3/

Comment 74

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86488/diff/2-3/

Comment 75

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87194/diff/2-3/

Comment 76

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/4-5/

Comment 77

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/3-4/

Comment 78

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87194/diff/3-4/

Comment 79

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/5-6/

Comment 80

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/4-5/

Comment 81

[Dão Gottwald [:dao]]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Better return after tabbrowser.removeTab(tab); and throw at the end of closeBrowser.

Comment 82

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87194/diff/4-5/

Comment 83

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/6-7/

Comment 84

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/5-6/

Comment 85

[Samael Wang [:freesamael] (away for now)]

(In reply to Dão Gottwald [:dao] from comment #81)
> Comment on attachment 8802950 [details]
> Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.
> 
> Better return after tabbrowser.removeTab(tab); and throw at the end of
> closeBrowser.

Updated. Thanks!

Comment 86

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801894 [details]
Bug 1276553 - Part 1: Make nsISHistory support partial mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86486/diff/3-4/

Comment 87

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801895 [details]
Bug 1276553 - Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86488/diff/3-4/

Comment 88

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87194/diff/5-6/

Comment 89

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801015 [details]
Bug 1276553 - Part 4: Implement frameloader level GroupedSHistory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85820/diff/7-8/

Comment 90

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8801896 [details]
Bug 1276553 - Part 5: Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86490/diff/6-7/

Comment 91

[Samael Wang [:freesamael] (away for now)]

Dao, do you mind to mark r+ at mozreview side? Not sure how mozreview works but it apparently won't automatically sync from bugzilla. I rebase the patch on mozilla central and upload, now it's marked r? again.

Comment 92

[Dão Gottwald [:dao]]

Comment on attachment 8802950 [details]
Bug 1276553 - Part 3: Add groupedSHistory related functions to nsIBrowser.

Sorry, I've burned too much time on the mozreview UI, plus it's my day off. Marking r+ here again.

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09b9bd35cf63
Part 1: Make nsISHistory support partial mode. r=smaug
https://hg.mozilla.org/integration/autoland/rev/97ff05c0dc7d
Part 2: Move nsIBrowser to dom/interfaces/base. r?=smaug
https://hg.mozilla.org/integration/autoland/rev/3bac8b1f4cd4
Part 3: Add groupedSHistory related functions to nsIBrowser. r=smaug
https://hg.mozilla.org/integration/autoland/rev/97f2b5e98b9c
Part 4: Implement frameloader level GroupedSHistory. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1c508ae66023
Part 5: Add test case. r=smaug

Comment 94

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/09b9bd35cf63
https://hg.mozilla.org/mozilla-central/rev/97ff05c0dc7d
https://hg.mozilla.org/mozilla-central/rev/3bac8b1f4cd4
https://hg.mozilla.org/mozilla-central/rev/97f2b5e98b9c
https://hg.mozilla.org/mozilla-central/rev/1c508ae66023