Closed Bug 539076 Opened 11 years ago Closed 11 years ago

Better error checking and lazy statements for bookmarks service

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mak, Assigned: mak)

Details

(Whiteboard: [Ts])

Attachments

(5 files, 1 obsolete file)

Get better error checking and kick old 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.
Attachment #421301 - Flags: review?(dietrich)
oh there is a failure in test_placesTxn, looks like we try to set an empty keyword, i'll check that.
Attachment #421301 - Attachment is obsolete: true
Attachment #421301 - Flags: review?(dietrich)
Comment on attachment 421301 [details] [diff] [review]
Part1: better error checking and introduce lazy statements code.

my fault, new patch coming.
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
Attachment #421435 - Flags: review?(dietrich)
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?!
you asked me this renaming some time ago, let's do it.
Attachment #421463 - Flags: review?(dietrich)
Attachment #421463 - Attachment description: rename ENUMERATE_OBSERVERS to NOTIFY_OBSERVERS → Part4: rename ENUMERATE_OBSERVERS to NOTIFY_OBSERVERS
this statement can be easily replaced
Attachment #421469 - Flags: review?(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!
Attachment #421356 - Flags: review?(dietrich) → review+
Attachment #421435 - Flags: review?(dietrich) → review+
Attachment #421459 - Flags: review?(dietrich) → review+
Attachment #421463 - Flags: review?(dietrich) → review+
Attachment #421469 - Flags: review?(dietrich) → review+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Windows Xpcshell and X0 machines were passing.
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.
and Linux is happy too, here.
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
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
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.