930269 - Clean up session store restoration

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

These are some patches to make the restoration side of session store a little less weird.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cleanup-restore-history

The main effect of this patch is to change the restoreHistory function to take a single tab rather than a list of tabs. The code for iterating over the list of tabs is moved to restoreHistoryPrecursor. My goal here is to make restoreHistory really simple, so that it just deals with restoring the history for one tab. That way, it will be easy to eventually move this functionality to a content script.

There is one significant change here that I want to mention. Currently, we use a single map for all tabs to track docshell IDs and bfcache entries. That means that if two different tabs share a bfcache entry when we save the session, then they'll continue to share the entry when we restore.

Using the same map for all tabs is problematic for e10s because the tabs might end up in different processes. I talked to Tim about this issue and it doesn't seem very important to share in this case. It seems like the only way you can end up sharing entries across tabs is by duplicating a tab. The UI for this isn't very well known, so it's probably used somewhat rarely. And when it is used, it doesn't seem very important that we do the sharing when we reload the session. So the patch uses a different map for each tab.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: restore-tab-content

This patch renames restoreTab to restoreTabContent. Given that restoreTab is only one part of restoring a tab (it just handles doing the load--it doesn't deal with history or scroll data or anything), I think it should have a less generic name.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: restore-tabs

This patch renames restoreHistoryPrecursor to restoreTabs. restoreHistoryPrecursor is the main entry point for restoration, so I think it should have a better name that suggests it significance.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move-tab-attr-stuff

This patch moves some code for restoring tab attributes from restoreHistory to restoreTabs. It doesn't make sense to me that it's in restoreHistory since it has nothing to do with history.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-history-read

This patch renames some functions in SessionHistory.jsm. I'd like to settle on a naming convention for these modules where the verbs are "collect" and "restore". This patch renames the collection code in SessionHistory from |read| to |collect|.

Also, I took off some of the underscores for internal methods. Given that we already have a separation between SessionHistory and SessionHistoryInternal, I don't see any reason to use underscores as well.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: history-restore

This patch moves most of the restoreHistory code to SessionHistory.jsm where it belongs. It also moves deserializeHistoryEntry there, since the serialization code is already there.

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 821337 [details] [diff] [review]
cleanup-restore-history

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2591,4 @@
>     *        Flag to indicate whether the given set of tabs aTabs should be
>     *        restored/loaded immediately even if restore_on_demand = true
>     */
> +  restoreHistory: function (window, tab, tabData, restoreImmediately) {

Personally, I'd prefer we update the argument uses in the method, and keep the convention for the argument names, e.g. aWindow, aTab, etc.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 821340 [details] [diff] [review]
restore-tab-content

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

Just the one thing, otherwise LGTM.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2665,5 @@
>     *        the tab to restore
>     *
>     * @returns true/false indicating whether or not a load actually happened
>     */
> +  restoreTabContent: function (aTab) {

Why did you drop "ssi_restoreTab" instead of making it "ssi_restoreTabContent"?

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 821341 [details] [diff] [review]
restore-tabs

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

LGTM, just the function name thing again.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2473,5 @@
>     * @param aRestoreImmediately
>     *        Flag to indicate whether the given set of tabs aTabs should be
>     *        restored/loaded immediately even if restore_on_demand = true
>     */
> +  restoreTabs: function (aWindow, aTabs, aTabData, aSelectTab,

Again, you dropped the function name. Apparently stack traces will magically know the method name now, so these aren't needed, which is why you're removing them I guess.

To me it seems a little inconsistent having some left in. Maybe we should maintain the current style and if we decide to, remove them all? (This might be a good idea since I've been told it doesn't *always* work)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 821337 [details] [diff] [review]
cleanup-restore-history

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2591,4 @@
>     *        Flag to indicate whether the given set of tabs aTabs should be
>     *        restored/loaded immediately even if restore_on_demand = true
>     */
> +  restoreHistory: function (window, tab, tabData, restoreImmediately) {

I really dislike the aSomeThing way of naming arguments but I agree with Steven, it seems unnecessary to lose blame information here.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 821348 [details] [diff] [review]
history-restore

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

::: browser/components/sessionstore/src/SessionHistory.jsm
@@ +322,5 @@
> +  deserializeEntry: function (entry, idMap, docIdentMap) {
> +
> +    function makeURI(aString) {
> +      return Services.io.newURI(aString, null, null);
> +    }

This should use Utils.makeURI().

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> I really dislike the aSomeThing way of naming arguments but I agree with
> Steven, it seems unnecessary to lose blame information here.

I tried making this change, but it actually makes the blame problem worse. If we use the aParam convention in this function, then |tab| and |tabData|, which formerly were local vars and are now params, need to be changed to |aTab| and |aTabData|. That will "damage" the blame history for 20 lines that would otherwise be unchanged. If we don't use the |aParam| convention, then only 3 lines will be "damaged" in the blame history that would otherwise be unchanged.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Tim, can you make a ruling about the style issues here? I'm willing to adopt almost any style, but I do have my own opinions.

I think that the aParam names are unnecessary and they cause pointless code churn during refactorings like this one. I honestly can't think what the original purpose for them might have been.

Regarding the ssi_funName stuff, the function name inference stuff in the JS engine is pretty powerful now. The only time it makes sense to use explicitly give the name is for anonymous functions passed to setTimeout and such, where there would otherwise be no name to infer. The other purpose for giving the name is when the function is recursive or when you want pass it to removeEventListener or something.

The reason I removed the ssi_funName stuff is that it makes the code harder to read in my opinion. It's just extra characters that cause lines to overflow and that your eyes have to scan past.

I'm also a big believer in gradual, incremental code changes. I don't think it makes sense to leave code in an inferior state just so that it looks like the code nearby.

Comment 14

[Steven MacLeod [:smacleod]]

(In reply to Bill McCloskey (:billm) from comment #13)
> I think that the aParam names are unnecessary and they cause pointless code
> churn during refactorings like this one. I honestly can't think what the
> original purpose for them might have been.

I have no strong feelings either way about aParam style names, I only mentioned them as to keep a consistent style throughout the file. If going forward we'd like to abandon that style here I'm fine with this being the start.


> Regarding the ssi_funName stuff, the function name inference stuff in the JS
> engine is pretty powerful now. The only time it makes sense to use
> explicitly give the name is for anonymous functions passed to setTimeout and
> such, where there would otherwise be no name to infer. The other purpose for
> giving the name is when the function is recursive or when you want pass it
> to removeEventListener or something.

Good to know, thanks.

> 
> The reason I removed the ssi_funName stuff is that it makes the code harder
> to read in my opinion. It's just extra characters that cause lines to
> overflow and that your eyes have to scan past.

Yup, if they're really unnecessary let's remove them when it makes sense and not add more. 

> 
> I'm also a big believer in gradual, incremental code changes. I don't think
> it makes sense to leave code in an inferior state just so that it looks like
> the code nearby.
 
I agree, especially with these ssi_funName's. In the case of style (e.g. the aParamName), consistency can greatly increase readability though. I'm fine with us incrementally moving towards a better style now that I know that's the plan here.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #12)
> I tried making this change, but it actually makes the blame problem worse.
> If we use the aParam convention in this function, then |tab| and |tabData|,
> which formerly were local vars and are now params, need to be changed to
> |aTab| and |aTabData|. That will "damage" the blame history for 20 lines
> that would otherwise be unchanged. If we don't use the |aParam| convention,
> then only 3 lines will be "damaged" in the blame history that would
> otherwise be unchanged.

Good point, I didn't think of that. Please ignore my comment then.

Regarding style, let's get rid of aParam argument names, let's not provide function names for stuff that the JS engine can figure out and let's move to all this whenever we touch a line or surrounding code, i.e. incrementally.

So, yeah, all what you two just said :)

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22a64c02375c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/39ce4f048683
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e58a9cfa1576
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/529ae5a54c32
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/14be7eb84433
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87bfb788a247

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22a64c02375c
https://hg.mozilla.org/mozilla-central/rev/39ce4f048683
https://hg.mozilla.org/mozilla-central/rev/e58a9cfa1576
https://hg.mozilla.org/mozilla-central/rev/529ae5a54c32
https://hg.mozilla.org/mozilla-central/rev/14be7eb84433
https://hg.mozilla.org/mozilla-central/rev/87bfb788a247

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> Regarding style, let's get rid of aParam argument names, let's not provide
> function names for stuff that the JS engine can figure out and let's move to
> all this whenever we touch a line or surrounding code, i.e. incrementally.
> 
> So, yeah, all what you two just said :)

This makes me happy.

Comment 19

[Ioana (away)]

Marking with verifyme for QA to do some exploratory on this component and ensure we have no regressions.

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

No regressions were found while doing exploratory testing on the Session Restore component using Firefox 28 beta 2 and Firefox 28 beta 3.

Marking this as verified.