559103 - custom distribution bookmark changes should be batched

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

it's a series of related bookmarks/folders/etc that are not batched.

attached patch performs the customizations in batch mode for better performance, and to ensure the operations happen in the correct order without interference from other operations that may be taking place.

Comment 1

[Dietrich Ayala (:dietrich)]

btw, i found this while looking into bug 523936. vaguely possible this is related.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 438790 [details] [diff] [review]
v1

Or Thunder's review here too.

Comment 3

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

the number of additions is small and hits just once, while i think this won't hurt, i also think won't be really useful (a batch causes all bookmarks queries to completely rebuild).
btw, instead of using self = this, you should pass this as second param to runInBatchMode, is not that the purpose of the param?

Comment 4

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

about the fact something could interrupt us without a batch, unless i miss something about how js work, i think while we are inside applyBookmarks other code cannot interrupt us.

Comment 5

[Dan Mills [:thunder]]

Comment on attachment 438790 [details] [diff] [review]
v1

Looks good to me!

Comment 6

[Dan Mills [:thunder]]

Even though we currently don't (as a policy), there isn't really an upper limit on how many items we might be adding there.  I don't think it's crazy to imagine adding as many as 20 or 30 (or more) items in there at some point.  I think batching would help in that case (? - I'm not really sure).

Also, perhaps a better argument, is that other unrelated code might be listening to places notifications, unaware that these actions can happen so early on during first-run.

Comment 7

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

(In reply to comment #6)
> Even though we currently don't (as a policy), there isn't really an upper limit
> on how many items we might be adding there.  I don't think it's crazy to
> imagine adding as many as 20 or 30 (or more) items in there at some point.  I
> think batching would help in that case (? - I'm not really sure).

Sure, everything is possible, but i have doubts that someone really wants to nag their users with 20 or 30 default bookmarks. I expect 3 or 4 bookmarks being the common case (in case of 20 or 30 additions, batching would help clearly).

> Also, perhaps a better argument, is that other unrelated code might be
> listening to places notifications, unaware that these actions can happen so
> early on during first-run.

We notify during batches too, it is up to the receiver to decide if them want to ignore those notifications or not. Actually this could be an argument for nsPlacesDBFlush.js (during a batch we don't flush), unfortunatly test_browserGlue_distribution.js already disables flushes, without any luck :(

Comment 8

[Dan Mills [:thunder]]

From previous experience, the only thing stopping custom distros from having more links is us forbidding it.  You'd be surprised what people want sometimes.

Adding Kev to cc list, he can provide more details on allowed/likely scenarios.  He tells me that currently we have a couple of distros with ~15 bookmarks each.  20 or 30 doesn't seem like such a stretch from that.

Comment 9

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

ok, i'm sold.
i think either pass "this" as param or directly let dc = DistributionCustomizer in the batch, i've recently discovered how "self" usage can be error prone.

Comment 10

[Dan Mills [:thunder]]

(In reply to comment #9)
> i've recently discovered how "self" usage can be error prone.

Can you elaborate, over email if you prefer? I'm curious...

Comment 11

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

just that i was changing a large piece of code inside a batch (in PlacesUIUtils) that was using self, and changing the external object (from a common object to a module object) caused me to do some find&replace of "self" because it was not valid anymore in a function closure. At that point discovering what each "self" was became a long and tedious manual work. Actually i think due to the size of the batch i ended up having 2 nested self definitions. So in cases like this one i think referring to the global module object is clearer.

Comment 12

[Dan Mills [:thunder]]

Ok, that makes sense, thanks.

Comment 13

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

Comment on attachment 438790 [details] [diff] [review]
v1

waiting for a patch without "self".

Comment 14

[Dietrich Ayala (:dietrich)]

Is this patch long dead?

Comment 15

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

yeah, actually I think it's wontfix, the number of distribution bookmarks is usually quite small (< 10), batching may be more expensive than not doing it.