Stop using localStorage in about:newTab

RESOLVED FIXED in Firefox 20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mak, Unassigned)

Tracking

unspecified
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

6 years ago
Whatever solution we go for about:home we should apply it to about:newTab, localStorage is bad for the common various reasons we well know (synchronous, mainthread and so on)
Created attachment 663721 [details] [diff] [review]
draft
Attachment #663721 - Flags: feedback?(mak77)
(Reporter)

Comment 2

6 years ago
Comment on attachment 663721 [details] [diff] [review]
draft

tim knows this code far better than me
Attachment #663721 - Flags: feedback?(mak77) → feedback?(ttaubert)
Comment on attachment 663721 [details] [diff] [review]
draft

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

Awesome, thank you for tackling this.

::: browser/modules/NewTabUtils.jsm
@@ +52,5 @@
> +    idbManager.initWindowless(this);
> +    let request = this.indexedDB.open("aboutnew", 1);
> +    request.onupgradeneeded = function(event) {
> +      event.target.result.createObjectStore("store");
> +    };

If "upgradeneeded" is fired and there's no DB yet, we need to migrate the old storage values contained in the localStorage. We need to keep this code around for at least one major version.

@@ +56,5 @@
> +    };
> +    request.onsuccess = function(event) {
> +      Storage.idb = event.target.result;
> +      aCallback();
> +    }

Instead of having Storage.idb contain the db we could for example add a method like 'Storage._getDatabase(aCallback)' that initializes the DB if necessary and calls the callback. (See also below.)

@@ +268,2 @@
>     */
> +  getLinks: function PinnedLinks_getLinks(aCallback) {

I need to think a little about how much stuff this will break :) Same for BlockedLinks.getLinks().

@@ +534,5 @@
> +
> +        Storage.init(function() {
> +          PinnedLinks.getLinks(callback);
> +          BlockedLinks.getLinks(callback);
> +        });

I think it would be better if the Storage would be initialized lazily when Storage.get/set/clear are called.
Attachment #663721 - Flags: feedback?(ttaubert)
I'll take a look at this
Assignee: nobody → andres
Andres, I was working on a patch here, if you want to take the bug let me know.
Oh sorry, I though it was stopped since it was unassigned. I'll assign you.
Assignee: andres → mar.castelluccio
(In reply to Andres Hernandez [:andreshm] from comment #6)
> Oh sorry, I though it was stopped since it was unassigned. I'll assign you.

Don't worry, it's my fault! If this is a high priority work, feel free to take it!
No longer blocks: 791447
Depends on: 791447
(Reporter)

Comment 8

6 years ago
Bug 791447 is basically addressing this by using prefs.
(In reply to Marco Bonardo [:mak] from comment #8)
> Bug 791447 is basically addressing this by using prefs.

Are prefs read using another thread?
I can't work on this bug in the near future, maybe I'll retake it during the Christmas holidays.
Assignee: mar.castelluccio → nobody
(Reporter)

Comment 10

6 years ago
(In reply to Marco Castelluccio [:marco] from comment #9)
> (In reply to Marco Bonardo [:mak] from comment #8)
> > Bug 791447 is basically addressing this by using prefs.
> 
> Are prefs read using another thread?

No, they are cached. Though the problem here was mostly a startup problem, since the about:home page was initing Storage and DOMStorage for a really simple task. This page has really just to store a couple values, prefs are fine for it, we should have just done that from the beginning.
(Reporter)

Comment 11

6 years ago
well, this bug as-is is fixed by bug 791447, since we no longer use localStorage. if we want to evaluate alternatives they can be filed elsewhere with the proposal and perf measurements.

Thanks everyone.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Attachment #663721 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.