Closed Bug 552444 Opened 10 years ago Closed 10 years ago

Move microsummaries service to Places toolkit

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: kairo, Assigned: mak)

References

Details

Attachments

(3 files, 7 obsolete files)

12.80 KB, patch
mano
: review+
Details | Diff | Splinter Review
4.24 KB, patch
Details | Diff | Splinter Review
43.95 KB, patch
Details | Diff | Splinter Review
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. ;-)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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?
Attached patch patch v1.0 (obsolete) — Splinter Review
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?
Attachment #432805 - Flags: feedback?(adw)
(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.
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 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.
Attachment #432805 - Flags: feedback?(adw)
(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
Attachment #433135 - Attachment description: Bug 552444 - Part 2: fix code style to Places toolkit, minor cleanup (1.1) → Part 2: fix code style to Places toolkit, minor cleanup (1.1)
Attachment #432822 - Attachment is obsolete: true
Attachment #433135 - Flags: review?(dietrich)
Attachment #433133 - Flags: review?(mano)
Attachment #433138 - Flags: review?(mano)
Attachment #433138 - Attachment description: Bug 552444 - Part 3: fix "UsrMicsumGens" dependency (1.1) → Part 3: fix "UsrMicsumGens" dependency (1.1)
Summary: Move microsummary service to toolkit → Move microsummary service to Places toolkit
Comment on attachment 433133 [details] [diff] [review]
Part 1: move microsummaries service to toolkit (1.1)

r=mano
Attachment #433133 - Flags: review?(mano) → review+
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.
Attachment #433138 - Flags: review?(mano) → review+
hm, looks like i broke something when splitting the first version of the patch... will check again, could be a bad split.
ok, looks like there is a problem with part 3
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.
Attachment #433138 - Attachment is obsolete: true
Comment on attachment 433135 [details] [diff] [review]
Part 2: fix code style to Places toolkit, minor cleanup (1.1)

needs unbitrot
Attachment #433135 - Attachment is obsolete: true
Attachment #433135 - Flags: review?(dietrich)
Comment on attachment 433331 [details] [diff] [review]
Part 2: fix "UsrMicsumGens" dependency (1.1)

This is now part 2
Attachment #433331 - Attachment description: Part 3: fix "UsrMicsumGens" dependency (1.1) → Part 2: fix "UsrMicsumGens" dependency (1.1)
Comment on attachment 433331 [details] [diff] [review]
Part 2: fix "UsrMicsumGens" dependency (1.1)

sorry Mano, i need a second pass.
Attachment #433331 - Flags: review?(mano)
Please, attach the right patches to bugs, lazy developer!
Attachment #433332 - Attachment is obsolete: true
Attachment #433338 - Flags: review?(dietrich)
Attachment #433332 - Flags: review?(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
Attachment #433338 - Flags: review?(dietrich) → review+
just a small glitch, did not remove the removed test name from tests array.
apart this tryserver did not show anything other.
Attachment #433331 - Attachment is obsolete: true
addressed comment
Attachment #433338 - Attachment is obsolete: true
Summary: Move microsummary service to Places toolkit → Move microsummaries service to Places toolkit
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Depends on: 554903
You need to log in before you can comment on or make changes to this bug.