552444 - Move microsummaries service to Places toolkit

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Robert Kaiser]

I started working on places bookmarks for SeaMonkey in bug 498596 and I wondered if I really needed to import another copy of the microsummary service into SeaMonkey application code.
Marco and Dietrich agreed that this service should move to toolkit, so here's the bug for it. ;-)

Comment 1

[Robert Kaiser]

As a note, my first patch for adding this to SeaMonkey has a part to add "UsrMicsumGens" to the directory provider, as the microsummary service needs that, can that be moved to toolkit as well?

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

asking for a first-pass to Drew since having more eyes on this is better, then will ask review to Dietrich.

While looking at this i noticed the sanitizeName function sucks, so i'm going to file a bug in searchService to replace it there as well.

Questions of the day:
1. Where are tests for Microsummaries???
2. Why i'm under the impression this component has been pushed in early beta stage?

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #1)
> As a note, my first patch for adding this to SeaMonkey has a part to add
> "UsrMicsumGens" to the directory provider, as the microsummary service needs
> that, can that be moved to toolkit as well?

uh, i missed that, yes, i should also solve that dependency.

Comment 4

[Marco Bonardo [:mak]]

Attachment: move usrMicSumGen folder definition

i will push both patched to try, to ensure everything is in place.
i did not remove the test in browser/components/dirprovider since it should be a valid test still.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 432805 [details] [diff] [review]
patch v1.0

Hmm, I guess the feedback flag is meant for overall direction, not nits about the patch?

In that case, I think you should split up your changes into two patches.  You're changing some of the service's implementation, while this bug simply says "move microsummary service to toolkit."  So, one small patch that moves, and then a bigger patch on top that makes style and implementation changes.  That way if there's a bug in your changes, they can be more easily backed out without reverting the move and status quo implementation.

That said, your changes look OK to me.  While you're changing things, though, you might want to replace that one QueryInterface definition with XPCOMUtils.generateQI.

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> (From update of attachment 432805 [details] [diff] [review])
> Hmm, I guess the feedback flag is meant for overall direction, not nits about
> the patch?

oh sorry, was about nits :)

> In that case, I think you should split up your changes into two patches. 

yeah, i will, cleanups in the service is independent and can be easily splitted off.

> That said, your changes look OK to me.  While you're changing things, though,
> you might want to replace that one QueryInterface definition with
> XPCOMUtils.generateQI.

i think that one is like that because nsIPrompt is conflicting so it needs a special QueryInterface management

Comment 7

[Marco Bonardo [:mak]]

Attachment: Part 1: move microsummaries service to toolkit (1.1)

Comment 8

[Marco Bonardo [:mak]]

Attachment: Part 2: fix code style to Places toolkit, minor cleanup (1.1)

Comment 9

[Marco Bonardo [:mak]]

Attachment: Part 3: fix "UsrMicsumGens" dependency (1.1)

Comment 10

[Asaf Romano (gone)]

Comment on attachment 433133 [details] [diff] [review]
Part 1: move microsummaries service to toolkit (1.1)

r=mano

Comment 11

[Asaf Romano (gone)]

Comment on attachment 433138 [details] [diff] [review]
Part 3: fix "UsrMicsumGens" dependency (1.1)

r=mano for the browser/ part. Ask bsmedberg for moa on the other parts of this patch.

Comment 12

[Marco Bonardo [:mak]]

hm, looks like i broke something when splitting the first version of the patch... will check again, could be a bad split.

Comment 13

[Marco Bonardo [:mak]]

ok, looks like there is a problem with part 3

Comment 14

[Marco Bonardo [:mak]]

Attachment: Part 2: fix "UsrMicsumGens" dependency (1.1)

This removes the key, after a brief discussion with Benjamin, looks like there is no advantage in having this key, we can just get the profile dir and attach to it (that is what we do for bookmarks backups). I'm using ProfDS just as an extra protection, since this service is used by importExport that runs early, profDS is guaranteed to be valid even before ProfD.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 433135 [details] [diff] [review]
Part 2: fix code style to Places toolkit, minor cleanup (1.1)

needs unbitrot

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 433331 [details] [diff] [review]
Part 2: fix "UsrMicsumGens" dependency (1.1)

This is now part 2

Comment 17

[Marco Bonardo [:mak]]

Attachment: Part 3: fix code style to Places toolkit, minor cleanup (1.2)

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 433331 [details] [diff] [review]
Part 2: fix "UsrMicsumGens" dependency (1.1)

sorry Mano, i need a second pass.

Comment 19

[Marco Bonardo [:mak]]

Attachment: Part 3: fix code style to Places toolkit, minor cleanup (1.2)

Please, attach the right patches to bugs, lazy developer!

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 433338 [details] [diff] [review]
Part 3: fix code style to Places toolkit, minor cleanup (1.2)

>   // The update interval as specified by the user (defaults to 30 minutes)
>   get _updateInterval() {
>-    var updateInterval =
>-      getPref("browser.microsummary.updateInterval", 30);
>+    var updateInterval = getPref("browser.microsummary.updateInterval", 30);
>     // the minimum update interval is 1 minute
>     return Math.max(updateInterval, 1) * 60 * 1000;
>   },

make the default a const and document it up at the top of the file with the others?

r=me

Comment 21

[Marco Bonardo [:mak]]

Attachment: Part 2: fix "UsrMicsumGens" dependency (1.2)

just a small glitch, did not remove the removed test name from tests array.
apart this tryserver did not show anything other.

Comment 22

[Marco Bonardo [:mak]]

Attachment: Part 3: fix code style to Places toolkit, minor cleanup (1.3)

addressed comment

Comment 23

[Marco Bonardo [:mak]]

Part 1: http://hg.mozilla.org/mozilla-central/rev/384e9d10da91
Part 2: http://hg.mozilla.org/mozilla-central/rev/9ad0cee7811b
Part 3: http://hg.mozilla.org/mozilla-central/rev/31176eb8917b