Clean up session store restoration

VERIFIED FIXED in Firefox 28

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
Firefox 28
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

These are some patches to make the restoration side of session store a little less weird.
Created attachment 821337 [details] [diff] [review]
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.
Attachment #821337 - Flags: review?(smacleod)
Created attachment 821340 [details] [diff] [review]
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.
Attachment #821340 - Flags: review?(smacleod)
Created attachment 821341 [details] [diff] [review]
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.
Attachment #821341 - Flags: review?(smacleod)
Created attachment 821342 [details] [diff] [review]
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.
Attachment #821342 - Flags: review?(smacleod)
Created attachment 821347 [details] [diff] [review]
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.
Attachment #821347 - Flags: review?(smacleod)
Created attachment 821348 [details] [diff] [review]
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.
Attachment #821348 - Flags: review?(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.
Attachment #821337 - Flags: review?(smacleod) → review+
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"?
Attachment #821340 - Flags: review?(smacleod) → review+
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)
Attachment #821341 - Flags: review?(smacleod) → review+
Attachment #821342 - Flags: review?(smacleod) → review+
Attachment #821347 - Flags: review?(smacleod) → review+
Attachment #821348 - Flags: review?(smacleod) → review+
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 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().
(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.
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.
Flags: needinfo?(ttaubert)
(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.
(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 :)
Flags: needinfo?(ttaubert)
(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

5 years ago
Marking with verifyme for QA to do some exploratory on this component and ensure we have no regressions.
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.