790178 - Stop using localStorage in about:newTab

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

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)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: draft

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 663721 [details] [diff] [review]
draft

tim knows this code far better than me

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Andres Hernandez [:andreshm]]

I'll take a look at this

Comment 5

[Marco Castelluccio [:marco]]

Andres, I was working on a patch here, if you want to take the bug let me know.

Comment 6

[Andres Hernandez [:andreshm]]

Oh sorry, I though it was stopped since it was unassigned. I'll assign you.

Comment 7

[Marco Castelluccio [:marco]]

(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!

Comment 8

[Marco Bonardo [:mak]]

Bug 791447 is basically addressing this by using prefs.

Comment 9

[Marco Castelluccio [:marco]]

(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.

Comment 10

[Marco Bonardo [:mak]]

(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.

Comment 11

[Marco Bonardo [:mak]]

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.