Stuff found during sessionstore code review

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Session Restore
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

48 Branch
Firefox 49
Points:
2
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Since I'm going to be largely responsible for this component, I went and read the source code.
Whilst reading I found a few nits, inconsistencies and obsolete code, which I thought would be nice to address.

A very small-scale cleanup, if you will.
Flags: qe-verify-
Flags: firefox-backlog+
(Assignee)

Comment 1

2 years ago
Created attachment 8758183 [details] [diff] [review]
PrivacyFilter.jsm and PrivacyLevel.jsm nits
Attachment #8758183 - Flags: review?(ttaubert)
(Assignee)

Comment 2

2 years ago
Created attachment 8758184 [details] [diff] [review]
RunState.jsm nits
Attachment #8758184 - Flags: review?(ttaubert)
(Assignee)

Comment 3

2 years ago
Created attachment 8758186 [details] [diff] [review]
TabAttributes.jsm and TabState.jsm nits
Attachment #8758186 - Flags: review?(ttaubert)
Attachment #8758184 - Flags: review?(ttaubert) → review+
Comment on attachment 8758186 [details] [diff] [review]
TabAttributes.jsm and TabState.jsm nits

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

::: browser/components/sessionstore/TabState.jsm
@@ -162,5 @@
>      }
>  
>      // The caller may explicitly request to omit privacy checks.
>      let includePrivateData = options && options.includePrivateData;
> -    let isPinned = tabData.pinned || false;

"pinned" is a boolean already, we just weren't sure whether the property exists, and we didn't want to see stupid errors in the console. I'm not sure the new version would prevent those.
Attachment #8758186 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 6

2 years ago
(In reply to Tim Taubert [:ttaubert] from comment #5)
> "pinned" is a boolean already, we just weren't sure whether the property
> exists, and we didn't want to see stupid errors in the console. I'm not sure
> the new version would prevent those.

> (function() { "use strict"; let o = {}; return !!o.pinned; })();
> false

IOW: no JS errors/ warning in this case. Thanks goodness. ;-)
Comment on attachment 8758183 [details] [diff] [review]
PrivacyFilter.jsm and PrivacyLevel.jsm nits

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

::: browser/components/sessionstore/PrivacyLevel.jsm
@@ +36,5 @@
> +   */
> +  check: function (url) {
> +    if (!url) {
> +      return false;
> +    }

The only two callers we have are definitely passing a valid URL. I'd leave this out so that callers are aware they're doing something they probably don't want to do.
Attachment #8758183 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Tim Taubert [:ttaubert] from comment #7)
> The only two callers we have are definitely passing a valid URL. I'd leave
> this out so that callers are aware they're doing something they probably
> don't want to do.

Of course! Thanks for the reviews, Tim! It was a joy to read it - the flow is quite easy to grasp.

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/023169ce92ef
https://hg.mozilla.org/mozilla-central/rev/52e0f82a635e
https://hg.mozilla.org/mozilla-central/rev/5ca8ea4f429a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.