Closed
Bug 552444
Opened 13 years ago
Closed 13 years ago
Move microsummaries service to Places toolkit
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: kairo, Assigned: mak)
References
Details
Attachments
(3 files, 7 obsolete files)
12.80 KB,
patch
|
asaf
:
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 | ||
Updated•13 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
![]() |
Reporter | |
Comment 1•13 years ago
|
||
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?
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
(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.
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
(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
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #432805 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
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)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #432822 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #433135 -
Flags: review?(dietrich)
Assignee | ||
Updated•13 years ago
|
Attachment #433133 -
Flags: review?(mano)
Assignee | ||
Updated•13 years ago
|
Attachment #433138 -
Flags: review?(mano)
Assignee | ||
Updated•13 years ago
|
Attachment #433138 -
Attachment description: Bug 552444 - Part 3: fix "UsrMicsumGens" dependency (1.1) → Part 3: fix "UsrMicsumGens" dependency (1.1)
Assignee | ||
Updated•13 years ago
|
Summary: Move microsummary service to toolkit → Move microsummary service to Places toolkit
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
hm, looks like i broke something when splitting the first version of the patch... will check again, could be a bad split.
Assignee | ||
Comment 13•13 years ago
|
||
ok, looks like there is a problem with part 3
Assignee | ||
Comment 14•13 years ago
|
||
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
Assignee | ||
Comment 15•13 years ago
|
||
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)
Assignee | ||
Comment 16•13 years ago
|
||
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)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #433332 -
Flags: review?(dietrich)
Assignee | ||
Comment 18•13 years ago
|
||
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)
Assignee | ||
Comment 19•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #433331 -
Flags: review?(mano) → review+
Comment 20•13 years ago
|
||
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+
Assignee | ||
Comment 21•13 years ago
|
||
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
Assignee | ||
Comment 22•13 years ago
|
||
addressed comment
Attachment #433338 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Summary: Move microsummary service to Places toolkit → Move microsummaries service to Places toolkit
Assignee | ||
Comment 23•13 years ago
|
||
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: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
You need to log in
before you can comment on or make changes to this bug.
Description
•