Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix documentation not showing on localStorage error #81964

Merged
merged 2 commits into from
Feb 12, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 8 additions & 23 deletions src/librustdoc/html/static/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,35 +89,20 @@ function hasOwnProperty(obj, property) {
return Object.prototype.hasOwnProperty.call(obj, property);
}

function usableLocalStorage() {
// Check if the browser supports localStorage at all:
if (typeof Storage === "undefined") {
return false;
}
// Check if we can access it; this access will fail if the browser
// preferences deny access to localStorage, e.g., to prevent storage of
// "cookies" (or cookie-likes, as is the case here).
try {
return window.localStorage !== null && window.localStorage !== undefined;
} catch(err) {
// Storage is supported, but browser preferences deny access to it.
return false;
}
}

function updateLocalStorage(name, value) {
if (usableLocalStorage()) {
localStorage[name] = value;
} else {
// No Web Storage support so we do nothing
try {
window.localStorage.setItem(name, value);
} catch(e) {
// localStorage is not accessible, do nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to attempt to call localStorage.clear() here in case enough data accumulated to reach the per-site limit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you want to remove all your data if you can't add more?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's assuming something filled up the storage to its limit which prevents storing new data, e.g. due to previous iterations of the site.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We store very little amount of information in the local storage (less than 40 bytes iirc), so we should be fine. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be discussed outside of the context of this pull request ?
If new behavior needs to be introduced, it can be independently of this bug fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, if there's nothing that could cause unbounded growth then the limitation is probably due to something else, e.g. a browser simply having localstorage disabled, clear() wouldn't help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we good to merge, then ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already approved it so it should be merged in the next days.

}
}

function getCurrentValue(name) {
if (usableLocalStorage() && localStorage[name] !== undefined) {
return localStorage[name];
try {
return window.localStorage.getItem(name);
} catch(e) {
return null;
}
return null;
}

function switchTheme(styleElem, mainStyleElem, newTheme, saveTheme) {
Expand Down