1015633 - Use the shared asynchronous db connection in nsLivemarksService

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

Attachment: patch.diff

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 8428285 [details] [diff] [review]
patch.diff

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

missing a piece!

::: toolkit/components/places/nsLivemarkService.js
@@ +99,5 @@
> +      row => {
> +        let id = row.getResultByName("id");
> +        let siteURL = row.getResultByName("siteURI");
> +        let guid = row.getResultByName("guid");
> +        livemarkSvc._livemarks[id] =

the old code was doing let livemarkSvc = this;

this code can't work as-is since livemarkSvc is undefined... Did this pass tests?

Comment 2

[Asaf Romano (gone)]

I ran test_mozIAsyncLivemarks and it passed, but maybe I broke it with some last minute change. I'll check

Comment 3

[Asaf Romano (gone)]

Unfortunately test_mozIAsyncLivemarks indeed passes with this patch. :-/

Comment 4

[Asaf Romano (gone)]

Not that we can do much about it - there are fall-backs, and the cache by itself is not exposed.

Comment 5

[Asaf Romano (gone)]

"If a non-StopIteration exception is thrown by the onRow handler, the exception is logged and processing of subsequent rows occurs as if nothing happened. The promise is still resolved (not rejected)."

Comment 6

[Asaf Romano (gone)]

Also note that even if the exception were propagated, the cache is empty in our tests, so the code in question is never executed.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

I've verified this works (manually). Automated test require us to have a non-empty cache and a way of accessing the cache from the test.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8428806 [details] [diff] [review]
patch

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

So to test the cache we should add a livemark before initing the livemarks service...
Something like "add fake livemark (folder with feeduri anno) through the bookmarks service, try to remove it through the livemarks service".
Should not be too hard, sure it's not exactly a clean approach. On the other side the livemarks service is also an nsIObserver, so we could easily pass a custom topic and on observing it, the service may clear the cache and enforce its reload (by calling again this._cacheReadyPromise = this._ensureAsynchronousCache().then(null, Cu.reportError);).
Maybe file a mentored bug to investigate a test covering that piece of code? I'd suggest the second approach.

Comment 9

[Asaf Romano (gone)]

Marco, see my previous comments. There are three different problems with testing this:
1) The cache is empty.
2) Even if was, we have fall-backs for "missing" cache entries.
3) Exceptions in onRow are ignored by Sqlite.jsm.

Comment 10

[Asaf Romano (gone)]

(In 2, I meant "event if it *wasn't*)

I filed bug 1016163.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #9)
> Marco, see my previous comments. There are three different problems with
> testing this:
> 1) The cache is empty.

I don't think this is a problem, if we provide a way to create a livemark and reset the cache, as I suggested.

> 2) Even if was, we have fall-backs for "missing" cache entries.

which fallbacks? we add stuff to the cache, but we don't have any fallback for stuff that WAS in the cache and has been cleared

Comment 12

[Asaf Romano (gone)]

You're right indeed. Let's take this to bug 1016163 though. This patch doesn't make the test situation worse.

Comment 13

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/2675e3bbdc06

Comment 14

[Marco Bonardo [:mak]]

Mano, please note the issue in bug 1015629 that caused a backout. I don't think it's worth a backout here (I suspect the issue is only on shutdown), but we need to solve that before the merge.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2675e3bbdc06