470429 - nsPlacesDBFlush::onItemAdded should not sync if the item is a container

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we are doing more work than needed, onItemAdded should sync places only if we create an uri item, containers don't use moz_places table

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

apart this, i was thinking that if we are in a batch we should not sync onItemAdded and onItemChanged... but i fear that could cause issues for longer batches, so for now i'm not doing that.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

a minor style coherence nit

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Created an attachment (id=353974) [details]
> patch
> 
> apart this, i was thinking that if we are in a batch we should not sync
> onItemAdded and onItemChanged... but i fear that could cause issues for longer
> batches, so for now i'm not doing that.
How would that cause issues for longer batches?  We don't want to do any syncing during batches right?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #3)
> (In reply to comment #1)
> How would that cause issues for longer batches?  We don't want to do any
> syncing during batches right?

if we crash in the middle of a batch, we could end up with lot of orphan bookmarks. suppose that the batch is an undo of the deletion of 100 bookmarks, hwv i think for that could make sense go with a followup

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> if we crash in the middle of a batch, we could end up with lot of orphan
> bookmarks. suppose that the batch is an undo of the deletion of 100 bookmarks,
> hwv i think for that could make sense go with a followup
And we should be doing that from within a transaction, so if we crash, sqlite will roll that back when we next open the database.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

ok, i'm sold

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 354134 [details] [diff] [review]
patch v2

r=sdwilsh

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

test_history_removeAllPages.js needs to be fixed contextually to this push since it relies on sync count

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.1

additional test fix, ready to land

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/c4785f2136df

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 355780 [details] [diff] [review]
patch v2.1

Low risk; small scope - reduces the number of times we sync to disk

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355780 [details] [diff] [review]
patch v2.1

a191=beltzner

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/442af45a96ec