539076 - Better error checking and lazy statements for bookmarks service

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Get better error checking and kick old code.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Part1: better error checking and introduce lazy statements code.

I'll do in 2 parts, this is big enough though (and sadly). In next part i'll convert old statements to use the lazy statements code.

Comment 2

[Marco Bonardo [:mak]]

oh there is a failure in test_placesTxn, looks like we try to set an empty keyword, i'll check that.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 421301 [details] [diff] [review]
Part1: better error checking and introduce lazy statements code.

my fault, new patch coming.

Comment 4

[Marco Bonardo [:mak]]

Attachment: Part1: better error checking and introduce lazy statements code.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Part2: Convert current global statements to lazy statements

This one converts 1:1 the old statements

I'll probably provide a third part, i'd like to make a new macro
GET_AND_ASSIGN_LAZY_STATEMENT(stmt, mDBStmt); that will

mozIStorageStatement* stmt = GetStatement(mDBStmt);
NS_ENSURE_STATE(stmt);
mozIStorageScoper scoper(stmt);

since this code is repeated in various services now.
and probably make the local macro in GetStatement be a global macro in Helpers.h

Comment 6

[Marco Bonardo [:mak]]

Also, while touching this i've noticed how many statements we have in bookmarks service, sounds a bit crazy seeing them, what are we doing so complex?!

Comment 7

[Marco Bonardo [:mak]]

Attachment: Part3: consolidate lazy statements boilerplate in macros

Comment 8

[Marco Bonardo [:mak]]

Attachment: Part4: rename ENUMERATE_OBSERVERS to NOTIFY_OBSERVERS

you asked me this renaming some time ago, let's do it.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Part5: kill mDBGetItemIndex

this statement can be easily replaced

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 421356 [details] [diff] [review]
Part1: better error checking and introduce lazy statements code.

whew, that's a lot of cleanup. r=me!

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/14c20441c8d8
http://hg.mozilla.org/mozilla-central/rev/09058ef6478c
http://hg.mozilla.org/mozilla-central/rev/0ddf6c0dc572
http://hg.mozilla.org/mozilla-central/rev/3049f6d7bb50
http://hg.mozilla.org/mozilla-central/rev/9ec061626142

Comment 12

[Marco Bonardo [:mak]]

Backed out due to unexplainable failures in Linux and Max Xd boxes... they fail on RemoveItem, a negative or 0 id is passed to it... no idea what's up, vut looks like the same issue causing some orange...

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263647218.1263651393.26473.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263643733.1263646383.31164.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263645378.1263648688.26329.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263646914.1263649426.3072.gz

Comment 13

[Marco Bonardo [:mak]]

Windows Xpcshell and X0 machines were passing.

Comment 14

[Marco Bonardo [:mak]]

bah, sounds like was just memory corruption due to stupid casts. Sounds crazy that tryserver runs did not notice anything. Btw, i'll push again first 2 parts once i can build in Linux debug. Mac is happy afaict.

Comment 15

[Marco Bonardo [:mak]]

and Linux is happy too, here.

Comment 16

[Marco Bonardo [:mak]]

pushed again the first 2 parts, if they'll stick, i'll push the other ones, since the issue was related to part2 afaict.
http://hg.mozilla.org/mozilla-central/rev/afcae9cfb798
http://hg.mozilla.org/mozilla-central/rev/7a7d0da67610

Comment 17

[Marco Bonardo [:mak]]

yeah, was that, pushed remaining parts.
http://hg.mozilla.org/mozilla-central/rev/6c91fcc70f18
http://hg.mozilla.org/mozilla-central/rev/5e098d789ca0
http://hg.mozilla.org/mozilla-central/rev/4b8bf926f4e6