546370 - Hook up Places db maintenance (PlacesDBUtils.jsm) on 1.9.x

Status: RESOLVED FIXED

(Camino Graveyard :: History, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

On 1.9.x, Places has preventative maintenance tasks run via PlacesDBUtils.jsm; we should hook them up (the 1.9.2 megapatch packages the file, but we don't do anything with it).

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> (the 1.9.2 megapatch packages the file, but we don't do anything with it).

But the patches on bug 552239 won't have us packaging it; this bug will need to do it.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This sounds like something we need for 2.1.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

For instance, today I deleted a 4K of History items during a test, and the places.sqlite file remained at 6.1 MB, with only 13 history items left in it.

Comment 4

[Stuart Morgan]

Attachment: fix

This does all the packaging and hook-up. I also did a bit of re-arranging in the project source tree, which is why some components that have nothing to do with this are moving around.

To capture from irc discussions, two surprising things here:
1) The code we need is in a .jsm that has no component I could call from C++ (browser.js calls it in Applications That Matter), so I had to make a stupid little component for Camino just to call one function.
2) The internals of the maintenance code is based on a timer that lives in toolkit/mozapps/update, so we are including a component that sounds like we don't need, but really we do. Luckily all it appears to include when MOZ_UPDATE isn't set is the timer we need.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 567877 [details] [diff] [review]
fix

This has got to be one of the craziest hacks we've ever done to work with Gecko ;-)  Thanks for doing it!

>diff --git a/Camino.xcodeproj/project.pbxproj b/Camino.xcodeproj/project.pbxproj
>@@ -5125,16 +5169,10 @@
> 				008115420B180F35001CB3F0 /* libthebes.dylib */,
> 				F6BD638701B30F5301A962F7 /* libgkgfx.dylib */,
> 				3FE5B9FC068A090F0001CECD /* libmozz.dylib */,
>-				F5461CBB03AAC25C01A96660 /* nsProxyAutoConfig.js */,
>-				0098B11D0CAEA75700DA0F2F /* nsHandlerService.js */,
>-				B33032AA10AE15EA002FA0A7 /* nsLivemarkService.js */,
>-				B328DEF9114ECFBF004D2EDC /* nsPlacesDBFlush.js */,
>-				DE581AB411865B1A00BAEA05 /* pluginGlue.js */,
>-				C7DE8E150F30D14F00CD3872 /* nsUrlClassifierListManager.js */,
>-				C7DE8E160F30D14F00CD3872 /* nsUrlClassifierLib.js */,
>-				0098B17F0CAEA9CF00DA0F2F /* XPCOMUtils.jsm */,
>+				000B0E3F144B81EF006A4B8E /* caminojsbridge.js */,
> 				F5247C2F0228B5E7013DD99A /* Gecko Security */,
> 				F6BD645501B3167601A962F7 /* Gecko Res */,
>+				000B0F3D144E151B006A4B8E /* Gecko Modules */,
> 				F6BD64FB01B3184301A962F7 /* Gecko Pref Defaults */,
> 				F507BA530213C26501D93544 /* Gecko Profile Defaults */,
> 				F56F242202AC6F7101A967F3 /* Gecko Static Components */,

Why isn't caminojsbridge.js inside "Gecko Components" > "Components" with the other JS components you moved there?

>diff --git a/jsbridge/Makefile.in b/jsbridge/Makefile.in

>+MODULE = camino

I wonder if this should be different from the MODULE for caminosearch?  I have no idea how that particular line works, though, so let's just keep cargo-culting that line ;-)

>+# Install to dist/components/

dist/bin/components/ please; let's not cargo-cult errors we know how to correct ;-)

>diff --git a/jsbridge/caminojsbridge.js b/jsbridge/caminojsbridge.js

This file needs a license block: http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh

I didn't check any of the GeckXPCOM JS itself; I assume that since the feature works for you, the JS is right ;-)

>diff --git a/jsbridge/chIJavascriptBridge.idl b/jsbridge/chIJavascriptBridge.idl

>+/**
>+ * Bridge interface for calling into JS-only components (e.g., some utilities
>+ * used by browse.js, which Camino doesn't have.
>+ */

I don't like the parenthetical part of the comment (which, at the very least, needs to have a closing paren and s/browse.js/browser.js/, as it stands now).

Since at the moment this is only used for utilities for Places, say that.  Or something like "(e.g., some utilities used by Places and called from browser.js, which Camino doesn't have)."

It seems dumb that you have to wait 24 hours for Places to shrink your db if you've deleted a bunch of stuff from it, but I guess that's better than never having the db shrink (but I guess that's not the dumbest thing we've encountered in this bug) :P  I didn't wait up 24 hours to see if it actually works, though, but if the timer's sane, I guess I can come back to those profiles tomorrow and watch the db shrink.

Anyway, r=ardissone with those nits addressed.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> >diff --git a/jsbridge/caminojsbridge.js b/jsbridge/caminojsbridge.js
> 
> This file needs a license block:
> http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh

Er, that's wrong (and will break the JS); I didn't look closely enough.

Apparently JS uses a custom boilerplate format that's not listed on the boilerplate page, insanely enough for an org that loves JS files :P

It's the sh style with /* pre- and */ post-pended:

/*
# ***** BEGIN LICENSE BLOCK *****
# Version: MPL 1.1/GPL 2.0/LGPL 2.1
[...]
#
# ***** END LICENSE BLOCK *****
*/

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(FWIW, it looks like bug 570387 would have solved this mess for us in Gecko 2 :P )

Comment 8

[Stuart Morgan]

Attachment: v2

Comment 9

[Stuart Morgan]

Landed as http://hg.mozilla.org/camino/rev/0c150694dbaa

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, about the timer being sane…it might be sane, but the stuff it's built on is absolutely not sane in the slightest :(

[01:25am] sauron: ugh
[01:27am] sauron: places won't do maintenance unless the idle service tells it we've been idle for 10min
[01:27am] sauron: but the idle service isn't seeing if the app has been idle, but rather the entire OS
[01:28am] sauron: so unless you leave your computer doing absolutely nothing *and* awake for >10 min every day, this stuff *still* won't run :/

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Also, FYI & FWIW, there was ~10ms startup hit from this checkin, I guess creating/starting the timer :P

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

:sigh:

I forgot to have you add the new Makefile to makefile.sh, too :(

Comment 13

[Stuart Morgan]

I assume I should add it? What does that file do? I can't remember (if I ever knew), and the internet wasn't all that helpful.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #13)
> I assume I should add it?

Yes, but if you don't get to it before I spin tonight, I'll do it before that.

> What does that file do? I can't remember (if I
> ever knew), and the internet wasn't all that helpful.

Remembering my mento, it teaches the build system about our Makefiles, which in turn provides benefits such as shaving some time off of the build.

It's obviously not critical or the build would have failed, but we ought to do it for those benefits and good housekeeping ;-)

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #14)
> (In reply to Stuart Morgan from comment #13)
> > I assume I should add it?
> 
> Yes, but if you don't get to it before I spin tonight, I'll do it before
> that.

http://hg.mozilla.org/camino/rev/4174a4b93b57

Comment 16

[Stuart Morgan]

Thanks :)