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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Justin Wood (:Callek)]

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

[[e-mail]]

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

Comment 3

[Philip Chee]

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

[[e-mail]]

(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

[Justin Wood (:Callek)]

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

[Serge Gautherie (:sgautherie)]

[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

[Philip Chee]

> Failed to load XPCOM component: ...\components\nsPlacesExpiration.js

See Comment 4:
>>     Part10: Add a new expiration component
> Ah yes, component packaging. Sigh...

Comment 8

[Robert Kaiser]

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.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

(Ftr, all the test_places/expiration/* tests pass on my local non-packaged Windows opt build ;-))

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Port packaging part (= Part 10), Sort components/* entries

Comment 11

[Robert Kaiser]

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.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Port packaging part (= Part 10) [Checkin: Comment 15]

Av1, with comment 11 suggestion(s).

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-191) Support downgrading [Checkin: Comment 16]

Comment 14

[Robert Kaiser]

Attachment: Adjust SeaMonkey to places expiration changes

This patch should be adjusting everything of SeaMonkey expect the packaging that Serge seems to already be addressing.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 423171 [details] [diff] [review]
(Av2) Port packaging part (= Part 10)
[Checkin: Comment 15]


http://hg.mozilla.org/comm-central/rev/d46180d14937

Comment 16

[Serge Gautherie (:sgautherie)]

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

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 423173 [details] [diff] [review]
Adjust SeaMonkey to places expiration changes


You need "#ifndef MOZILLA_1_9_2_BRANCH" everywhere.!.

Comment 18

[Robert Kaiser]

(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

[Serge Gautherie (:sgautherie)]

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

Comment 20

[[e-mail]]

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

[Justin Wood (:Callek)]

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

[Jens Hatlak (:InvisibleSmiley)]

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>

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Port the non-UI part only for now

KaiRo's patch, without 2 UI files, but keeping MOZILLA_1_9_2_BRANCH support.

Comment 24

[Robert Kaiser]

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.

Comment 25

[Serge Gautherie (:sgautherie)]

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

Comment 26

[Robert Kaiser]

(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

[Serge Gautherie (:sgautherie)]

Attachment: (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".)

Comment 28

[Robert Kaiser]

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

Comment 29

[Robert Kaiser]

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.

Comment 30

[Serge Gautherie (:sgautherie)]

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

Comment 31

[Robert Kaiser]

(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

[Serge Gautherie (:sgautherie)]

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

Comment 33

[Robert Kaiser]

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!