Closed
Bug 536374
Opened 14 years ago
Closed 14 years ago
Places history changes due to async expiration. (Port bug 520165.)
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a1
People
(Reporter: mak, Assigned: kairo)
References
(Blocks 1 open bug)
Details
(Keywords: fixed-seamonkey2.0.3)
Attachments
(3 files, 3 obsolete files)
2.65 KB,
patch
|
kairo
:
review+
|
Details | Diff | Splinter Review |
921 bytes,
patch
|
kairo
:
review+
kairo
:
approval-seamonkey2.0.3+
|
Details | Diff | Splinter Review |
10.35 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
I'm bringing some changes to how Places manages history expiration in bug 520165. I'm noting here the most disruptive changes that could affect Seamonkey: 1. we won't use anymore the old history preferences (browser.history_expire_*) instead we will have a places.history.enabled bool pref. the history limit will be calculated based on available memory, database cache size and cpu cores. it is still possible to limit manually number of pages through an hidden places.history.expiration.max_pages pref. it is possible to change the cache size percentage through places.history.cache_per_memory_percentage. 2. RemoveAllPages will clear visits synchronously, but pages, annotations and other orphans will be cleared asynchronously. So removeAllPages won't be completely sync anymore (you can listen for "places-expiration-finished" notification in case, tests are/shouldbe using this approach). 3. onPageExpired notification is being renamed to onDeleteVisits (to be clearer about his reason to exist aside from onDeleteURI) I plan to land that bug soon, so in case is there any concern about the above choices, please contact me.
Updated•14 years ago
|
Version: unspecified → Trunk
Updated•14 years ago
|
Flags: blocking-seamonkey2.1a1?
Summary: Places history changes due to async expiration → Places history changes due to async expiration. (Port bug 520165.)
Comment 1•14 years ago
|
||
Robert, Marco here was great and filed a bug for us, and listed the likely issues. Your our expert on Places history so probably worth a look when you get a chance.
Comment 2•14 years ago
|
||
Only 1. affects us, which involves making changes to the pref panel.
![]() |
||
Comment 3•14 years ago
|
||
According to my notes as well as: Part7: Provide a new preference to toggle history. We will also need to port parts of: Part8: Change onPageExpired to onDeleteVisits (feedwriter.js). Part9: remove old expiration code. Part9 (Satchel): fix dependancies (?) Part10: Add a new expiration component
Comment 4•14 years ago
|
||
(In reply to comment #3) > According to my notes as well as: > Part7: Provide a new preference to toggle history. I was assuming that as part of my previous comment. > We will also need to port parts of: > Part8: Change onPageExpired to onDeleteVisits (feedwriter.js). We don't have feedwriter.js, do we? > Part9: remove old expiration code. > Part10: Add a new expiration component Ah yes, component packaging. Sigh...
Comment 5•14 years ago
|
||
> > We will also need to port parts of:
> > Part8: Change onPageExpired to onDeleteVisits (feedwriter.js).
> We don't have feedwriter.js, do we?
suite/feeds/src/FeedWriter.js
Comment 6•14 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20100118 SeaMonkey/2.1a1pre] (comm-central-trunk-win32/1263816855) (W2Ksp4) Ftr, { Failed to load XPCOM component: ...\components\nsPlacesExpiration.js }
![]() |
||
Comment 7•14 years ago
|
||
> Failed to load XPCOM component: ...\components\nsPlacesExpiration.js See Comment 4: >> Part10: Add a new expiration component > Ah yes, component packaging. Sigh...
![]() |
Assignee | |
Comment 8•14 years ago
|
||
I've tried around xpcshell test runs for SeaMonkey trunk recently, and this comes up: TypeError: Cc['@mozilla.org/places/expiration;1'] is undefined I believe that's just the missing packaging mentioned here.
Updated•14 years ago
|
Blocks: SmTestFail
Comment 9•14 years ago
|
||
(In reply to comment #8) (Ftr, all the test_places/expiration/* tests pass on my local non-packaged Windows opt build ;-))
Comment 10•14 years ago
|
||
Attachment #423168 -
Flags: review?(kairo)
![]() |
Assignee | |
Comment 11•14 years ago
|
||
Comment on attachment 423168 [details] [diff] [review] (Av1) Port packaging part (= Part 10), Sort components/* entries The core of the patch looks good to me, but I don't like the unrelated reordering. We really should put off the reordering into a separate bug/patch - esp. as hg annotate then points to that checkin.
Attachment #423168 -
Flags: review?(kairo) → review-
Comment 12•14 years ago
|
||
Av1, with comment 11 suggestion(s).
Attachment #423168 -
Attachment is obsolete: true
Attachment #423171 -
Flags: review?(kairo)
Comment 13•14 years ago
|
||
Attachment #423172 -
Flags: review?(kairo)
Attachment #423172 -
Flags: approval-seamonkey2.0.3?
![]() |
Assignee | |
Comment 14•14 years ago
|
||
This patch should be adjusting everything of SeaMonkey expect the packaging that Serge seems to already be addressing.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #423173 -
Flags: superreview?(neil)
Attachment #423173 -
Flags: review?(neil)
![]() |
Assignee | |
Updated•14 years ago
|
Attachment #423171 -
Flags: review?(kairo) → review+
![]() |
Assignee | |
Updated•14 years ago
|
Attachment #423172 -
Flags: review?(kairo)
Attachment #423172 -
Flags: review+
Attachment #423172 -
Flags: approval-seamonkey2.0.3?
Attachment #423172 -
Flags: approval-seamonkey2.0.3+
Comment 15•14 years ago
|
||
Comment on attachment 423171 [details] [diff] [review] (Av2) Port packaging part (= Part 10) [Checkin: Comment 15] http://hg.mozilla.org/comm-central/rev/d46180d14937
Attachment #423171 -
Attachment description: (Av2) Port packaging part (= Part 10) → (Av2) Port packaging part (= Part 10)
[Checkin: Comment 15]
Comment 16•14 years ago
|
||
Comment on attachment 423172 [details] [diff] [review] (Bv1-191) Support downgrading [Checkin: Comment 16] http://hg.mozilla.org/releases/comm-1.9.1/rev/179c9263c4d1
Attachment #423172 -
Attachment description: (Bv1-191) Support downgrading → (Bv1-191) Support downgrading
[Checkin: Comment 16]
Updated•14 years ago
|
Keywords: fixed-seamonkey2.0.3
Comment 17•14 years ago
|
||
Comment on attachment 423173 [details] [diff] [review] Adjust SeaMonkey to places expiration changes You need "#ifndef MOZILLA_1_9_2_BRANCH" everywhere.!.
![]() |
Assignee | |
Comment 18•14 years ago
|
||
(In reply to comment #17) > (From update of attachment 423173 [details] [diff] [review]) > > You need "#ifndef MOZILLA_1_9_2_BRANCH" everywhere.!. No, I will not go and ifdef XUL files, and it's not clear yet that 1.9.2 is even relevant for SeaMonkey. I see two possibilities: 1) Check this is now while 1.9.3 seems to be the target for 2.1 and eventually back it out on a comm-1.9.2 branch when it comes up. 2) Defer checkin to post-branch-creation time. I intentionally exclude possibility #3 which would be to make XUL files be #ifdef'ed and preprocessed, I intend to stay sane.
Comment 19•14 years ago
|
||
(In reply to comment #18) > 1) Check this is now while 1.9.3 seems to be the target for 2.1 and eventually > back it out on a comm-1.9.2 branch when it comes up. > 2) Defer checkin to post-branch-creation time. I would prefer #2 if that's planned to happen "soon" (there are other fixes in this case anyway :-[), otherwise I agree #1 would be the best we can do atm :-< > I intend to stay sane. I would intend to not break one tree to fix the other one: even if you don't do #3, my point was not to forget about that case.
Updated•14 years ago
|
Attachment #423173 -
Flags: review?(neil) → review+
Comment 20•14 years ago
|
||
Comment on attachment 423173 [details] [diff] [review] Adjust SeaMonkey to places expiration changes What's your plan on 1.9.2/3 support?
Comment 21•14 years ago
|
||
Robert, if we don't know when 1.9.2 is branching, I'm happy with filing a new bug blocking this one "If SeaMonkey 2.1 uses Gecko 1.9.2 Backout change from Bug 536374" and keep up there a (frequently) updated |hg backout| diff of this, so that we can easily fix once the branch happens. And keep said bug a blocker until we have a decision; I'd rather keep the trunk-trunk working for SM here.
Comment 22•14 years ago
|
||
This requires Help changes (for which a bug should be filed). Also I suggest adding a relnote explaining people that the old prefs allowed for making some false assumptions (e.g. only the minimum number of days to keep history could be set effectively since the upper bound really was determined by the maximum number of pages to keep). Cf. MaK's blog post: <http://blog.bonardo.net/2010/01/20/places-got-async-expiration>
Updated•14 years ago
|
Whiteboard: [needs c-1.9.2 branch]
Updated•14 years ago
|
Depends on: C192Branch
Comment 23•14 years ago
|
||
KaiRo's patch, without 2 UI files, but keeping MOZILLA_1_9_2_BRANCH support.
Attachment #426804 -
Flags: review?(neil)
![]() |
Assignee | |
Comment 24•14 years ago
|
||
Comment on attachment 426804 [details] [diff] [review] (Dv1) Port the non-UI part only for now Please just leave this bug alone. I don't want those ifdefs there, and I'll move along with it once we have a decision on where we are going.
Attachment #426804 -
Flags: review?(neil) → review-
Comment 25•14 years ago
|
||
(In reply to comment #24) > Please just leave this bug alone. I don't want those ifdefs there, and I'll In comment 18, you wanted to leave the XUL part (only) alone and my patch does that. > move along with it once we have a decision on where we are going. I really feel pissed off when you tell me you eventually want more work on c-c SeaMonkey builds and test failures but then block fixing what we can :-(
![]() |
Assignee | |
Comment 26•14 years ago
|
||
(In reply to comment #25) > (In reply to comment #24) > > > Please just leave this bug alone. I don't want those ifdefs there, and I'll > > In comment 18, you wanted to leave the XUL part (only) alone and my patch does > that. It does not follow any of the two options I laid out int hat comment. Also, this bug is assigned to me intentionally, if you don't want to trample on my work and ego, please ask me before touching any of it. > > move along with it once we have a decision on where we are going. > > I really feel pissed off when you tell me you eventually want more work on c-c > SeaMonkey builds and test failures but then block fixing what we can :-( How does this change test failures?
Comment 27•14 years ago
|
||
Dv1, with comment 26 suggestion(s). (In reply to comment #26) > It does not follow any of the two options I laid out int hat comment. Also, None of the 3 options actually: it's a 4th one. > this bug is assigned to me intentionally, if you don't want to trample on my > work and ego, please ask me before touching any of it. Trample? I'm just splitting it into 2 parts. Ego? Ah! Sorry, fixing that it beyond my Mozilla skill. Ask? I didn't touch your patch, just attached mine in addition... That said, if you simply want your name on my patch, that's no problem: just say that. > How does this change test failures? I expected this would fix bug 541746 (in addition to whatever good this would do to the build itself). I had not tested it and it turns out you're right that my patch, as is, would not have fixed it. Now, I've basically checked the tests and here's a new version of my patch: *'pref("browser.history_expire_days", 180);' needs to be removed so test_download_history.js succeeds :-) (Don't investigate, don't care.) *The pref UI still "works", the 3 values just appear as 0 :-| *test_frecency.js is still failing ftb :-( (I'll look at it "later".)
Attachment #426804 -
Attachment is obsolete: true
Attachment #426914 -
Flags: review?(neil)
![]() |
Assignee | |
Comment 28•14 years ago
|
||
(In reply to comment #27) > Trample? I'm just splitting it into 2 parts. You do. And you might succeed in driving me out of even more Mozilla work. The only way I can save my personality seems to not do Mozilla work recently. I have already thrown away one piece of work I invested hours in, I may abandon more if you continue to set foot in this bug. If you want to take ownership of history and places overall in SeaMonkey, feel free to do so, but don't expect my help. > I expected this would fix bug 541746 (in addition to whatever good this would > do to the build itself). > I had not tested it and it turns out you're right that my patch, as is, would > not have fixed it. So then leave it alone, will you?
![]() |
Assignee | |
Comment 29•14 years ago
|
||
Comment on attachment 426914 [details] [diff] [review] (Dv2) Port the non-UI part only for now [Not relevant anymore] No need to either take away mostly reviewed work from me or introduce any ifdefs in this area at this stage. Please just leave this bug alone.
Attachment #426914 -
Flags: review?(neil) → review-
Comment 30•14 years ago
|
||
(In reply to comment #28) > don't expect my help. Wow! You *do* have a problem with that one :-( Calm down... I can't begin to see the Mozilla relation between my _proposal_ to update 1+ prefs to fix a test++ and your talk about your personality, dumping your own work or module ownership. > So then leave it alone, will you? I will: I'm just very sad you see this as a "you or me". Please, keep doing your good work! I'll just try and keep not doing my (unwanted) one :-/
![]() |
Assignee | |
Comment 31•14 years ago
|
||
(In reply to comment #30) > Wow! You *do* have a problem with that one :-( Calm down... No, surely not with you, only with those patches here, which are redundant with a r+ patch that is already here. > > So then leave it alone, will you? > > I will: I'm just very sad you see this as a "you or me". It's best to leave bugs that are assigned and have a r+ patch alone. We are in discussions right now if we need 1.9.2 ifdefs in suite/ at all or not, and this is nothing that we need immediately from what I see, and for now, it better to investigate unknown test failures - if this is the last one left, I'm happy to go with a minimal fix for that, but I'd prefer to have you concentrate on problems where we don't have solutions around yet, and defer this one until we have a decision on 1.9.2/.3 strategy for SeaMonkey. That said, please absolutely keep up your very awesome work in those areas like buildsystem ports, unsolved test failures, etc. - this is absolutely cool and welcome to have!
Comment 32•14 years ago
|
||
(In reply to comment #31) > It's best to leave bugs that are assigned and have a r+ patch alone. I just meant not to have you spend time to make the branching patch... > it better to investigate unknown test failures - if this is the last one left, We just have different ways to work: I prefer to push the fixes we have then see what remains rather than diagnose bugs which will be stalled, as in there is not going to be any last failure until we fix some of them first :-|
Updated•14 years ago
|
Updated•14 years ago
|
Whiteboard: [needs c-1.9.2 branch]
Updated•14 years ago
|
Attachment #426914 -
Attachment description: (Dv2) Port the non-UI part only for now → (Dv2) Port the non-UI part only for now
[Not relevant anymore]
Attachment #426914 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #423173 -
Flags: superreview?(neil) → superreview+
![]() |
Assignee | |
Comment 33•14 years ago
|
||
Pushed attachment 423173 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/47792f7227eb This bug should be fixed now. Thanks again to Marco for filing it!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Updated•14 years ago
|
Target Milestone: --- → seamonkey2.1a1
![]() |
Assignee | |
Updated•14 years ago
|
Flags: blocking-seamonkey2.1a1?
You need to log in
before you can comment on or make changes to this bug.
Description
•