Places history changes due to async expiration. (Port bug 520165.)

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mak, Assigned: Robert Kaiser)

Tracking

(Blocks: 1 bug, {fixed-seamonkey2.0.3})

Trunk
seamonkey2.1a1
fixed-seamonkey2.0.3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Version: unspecified → Trunk
Blocks: 467530
Flags: blocking-seamonkey2.1a1?
Summary: Places history changes due to async expiration → Places history changes due to async expiration. (Port bug 520165.)
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

7 years ago
Only 1. affects us, which involves making changes to the pref panel.

Comment 3

7 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

7 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...
> > 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
[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

7 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

7 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.
Blocks: 452942
(In reply to comment #8)

(Ftr, all the test_places/expiration/* tests pass on my local non-packaged Windows opt build ;-))
Created attachment 423168 [details] [diff] [review]
(Av1) Port packaging part (= Part 10), Sort components/* entries
Attachment #423168 - Flags: review?(kairo)
(Assignee)

Comment 11

7 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-
Created attachment 423171 [details] [diff] [review]
(Av2) Port packaging part (= Part 10)
[Checkin: Comment 15]

Av1, with comment 11 suggestion(s).
Attachment #423168 - Attachment is obsolete: true
Attachment #423171 - Flags: review?(kairo)
Created attachment 423172 [details] [diff] [review]
(Bv1-191) Support downgrading
[Checkin: Comment 16]
Attachment #423172 - Flags: review?(kairo)
Attachment #423172 - Flags: approval-seamonkey2.0.3?
(Assignee)

Comment 14

7 years ago
Created attachment 423173 [details] [diff] [review]
Adjust SeaMonkey to places expiration changes

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

7 years ago
Attachment #423171 - Flags: review?(kairo) → review+
(Assignee)

Updated

7 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 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 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]
Keywords: fixed-seamonkey2.0.3
Blocks: 541746
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

7 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.
(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

7 years ago
Attachment #423173 - Flags: review?(neil) → review+
Comment on attachment 423173 [details] [diff] [review]
Adjust SeaMonkey to places expiration changes

What's your plan on 1.9.2/3 support?
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.
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>
Whiteboard: [needs c-1.9.2 branch]
Depends on: 544237
Created attachment 426804 [details] [diff] [review]
(Dv1) Port the non-UI part only for now

KaiRo's patch, without 2 UI files, but keeping MOZILLA_1_9_2_BRANCH support.
Attachment #426804 - Flags: review?(neil)
(Assignee)

Comment 24

7 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-
(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

7 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?
Created attachment 426914 [details] [diff] [review]
(Dv2) Port the non-UI part only for now
[Not relevant anymore]

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

7 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

7 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-
(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

7 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!
(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 :-|
Depends on: 546484
No longer depends on: 544237
Whiteboard: [needs c-1.9.2 branch]
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

7 years ago
Attachment #423173 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 33

7 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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 546936
(Assignee)

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a1
No longer blocks: 467530
(Assignee)

Updated

7 years ago
Flags: blocking-seamonkey2.1a1?
You need to log in before you can comment on or make changes to this bug.