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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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
(Assignee)

Updated

10 years ago
Blocks: 442967
(Assignee)

Comment 1

10 years ago
Created attachment 353974 [details] [diff] [review]
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.
Attachment #353974 - Flags: review?(sdwilsh)
(Assignee)

Comment 2

10 years ago
Created attachment 353975 [details] [diff] [review]
patch v1.1

a minor style coherence nit
Attachment #353974 - Attachment is obsolete: true
Attachment #353975 - Flags: review?(sdwilsh)
Attachment #353974 - Flags: review?(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?
(Assignee)

Comment 4

10 years ago
(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
(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.
(Assignee)

Comment 6

10 years ago
Created attachment 354134 [details] [diff] [review]
patch v2

ok, i'm sold
Attachment #353975 - Attachment is obsolete: true
Attachment #354134 - Flags: review?(sdwilsh)
Attachment #353975 - Flags: review?(sdwilsh)
Comment on attachment 354134 [details] [diff] [review]
patch v2

r=sdwilsh
Attachment #354134 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 8

10 years ago
test_history_removeAllPages.js needs to be fixed contextually to this push since it relies on sync count
(Assignee)

Comment 9

10 years ago
Created attachment 355780 [details] [diff] [review]
patch v2.1

additional test fix, ready to land
Attachment #354134 - Attachment is obsolete: true
(Assignee)

Comment 10

10 years ago
http://hg.mozilla.org/mozilla-central/rev/c4785f2136df
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1

Updated

10 years ago
Attachment #355780 - Flags: approval1.9.1?
Comment on attachment 355780 [details] [diff] [review]
patch v2.1

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

Updated

10 years ago
Flags: in-testsuite+
Attachment #355780 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.