Closed Bug 1015633 Opened 6 years ago Closed 6 years ago

Use the shared asynchronous db connection in nsLivemarksService

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mano, Assigned: mano)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch.diff (obsolete) — Splinter Review
No description provided.
Attachment #8428285 - Flags: review?(mak77)
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?
Attachment #8428285 - Flags: review?(mak77)
I ran test_mozIAsyncLivemarks and it passed, but maybe I broke it with some last minute change. I'll check
Unfortunately test_mozIAsyncLivemarks indeed passes with this patch. :-/
Not that we can do much about it - there are fall-backs, and the cache by itself is not exposed.
"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)."
Also note that even if the exception were propagated, the cache is empty in our tests, so the code in question is never executed.
Attached patch patchSplinter Review
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.
Attachment #8428285 - Attachment is obsolete: true
Attachment #8428806 - Flags: review?(mak77)
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.
Attachment #8428806 - Flags: review?(mak77) → review+
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.
(In 2, I meant "event if it *wasn't*)

I filed bug 1016163.
(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
You're right indeed. Let's take this to bug 1016163 though. This patch doesn't make the test situation worse.
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.
https://hg.mozilla.org/mozilla-central/rev/2675e3bbdc06
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.