373944 - revert the workarounds for bug #373719, bug #373721, bug #374150 and bug #374166 caused by bug #267833

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

revert the workarounds for bug #373719 and bug #373721 caused by bug #267833

bz writes:

We may want to do something similar to what scripts do and only fire
constructors from EndUpdate if they got posted during the update.  Not sure;
need to look over those stacks a little more (in particular, the reentry of
SetView on the tree there).

Unfortunately, I won't be able to do that until the 27th.  Were we going to
enable places before then, or is this blocking places development?  If so, it
might be worth doing the workaround for now....  :(

this bug tracks backing out the hacks i checked in for bug #373719 and bug #373721 (I'll attach a patch)

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch to back me out

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch 1 of 3 to back out

here's a patch to back me out, as the fix changed.

note, asaf suggests, we may not to back me out even if the regression is fixed.

Comment 3

[(not reading, please use [e-mail] instead)]

a couple more changes were needed as well, which we'd want to back out.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: patch 2 of 3 to back out.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch 3 of 3 to back out.

Comment 6

[Asaf Romano (gone)]

Also need to back out the upcoming change from bug 382101.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Asaf, Seth, are you guys going to do this?  Or should I try to?

Comment 8

[(not reading, please use [e-mail] instead)]

> Asaf, Seth, are you guys going to do this?  Or should I try to?

I think it makes more sense for us to do it.

taking for a6.

Comment 9

[(not reading, please use [e-mail] instead)]

accepting, hope to have a back out patch to confirm that now that bug #373756 is fixed, we don't need these workarounds.

hope to have that today, so that we dveditz/jonas/bz can backport the fix for bug #373756 and bug #267833 on the branch.

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: patch

looking at the bugs mentioned in this bug and the patches, here's all I could find that needed to be backed out.

Note, I checked all of these sources:

https://bugzilla.mozilla.org/attachment.cgi?id=266679, moveBookmarks.js
https://bugzilla.mozilla.org/attachment.cgi?id=258748, history-panel.js
https://bugzilla.mozilla.org/attachment.cgi?id=258744, bookmarkProperties.js
https://bugzilla.mozilla.org/attachment.cgi?id=258602, tree.xml / places.js

Comment 11

[Asaf Romano (gone)]

Comment on attachment 269043 [details] [diff] [review]
patch

r=mano

Comment 12

[(not reading, please use [e-mail] instead)]

fixed.

Checking in browser/components/places/content/moveBookmarks.js;
/cvsroot/mozilla/browser/components/places/content/moveBookmarks.js,v  <--  move
Bookmarks.js
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.90; previous revision: 1.89
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.74; previous revision: 1.73
done

Comment 13

[(not reading, please use [e-mail] instead)]

backing myself out, this causes a performance regression.  see bug #385208

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 269043 [details] [diff] [review]
patch

this patch causes performance problems.

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: venkman profiling (after the regression)

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: venkman profiling (before the regression)

Comment 17

[(not reading, please use [e-mail] instead)]

notice the extra call to load() and set_place() between those two venkman profiles.

I'm not sure fixing this bug while not regressing performance is going to happen for A6.

Comment 18

[(not reading, please use [e-mail] instead)]

not going to happen for m7, sliding.

Comment 19

[(not reading, please use [e-mail] instead)]

not UI related, so sliding again.

Comment 20

[Asaf Romano (gone)]

The extra-calls to load() (and thus the slowness) were due to the removal of the |this.place| check in the tree constructor.

Comment 21

[Asaf Romano (gone)]

Attachment: patch

Comment 22

[(not reading, please use [e-mail] instead)]

asaf, please make sure to test with a profile with a lot of bookmarks.

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 284706 [details] [diff] [review]
patch

the change looks fine. please keep an eye on the perf tinderboxes.

Comment 24

[Asaf Romano (gone)]

mozilla/browser/components/places/content/moveBookmarks.js 1.11
mozilla/browser/components/places/content/places.js 1.111
mozilla/browser/components/places/content/tree.xml 1.84

Comment 25

[pal-moz]

after this checkin, 20071024_1939_firefox-3.0a9pre.en-US.win32.zip

open Places Organizer,
in default Error Console,

Error: [Exception... "Component returned failure code: 0x804b000a [nsIIOService.newURI]"  nsresult: "0x804b000a (<unknown>)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsScriptableIO.js :: ScriptableIO_newURI :: line 229"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsScriptableIO.js
Line: 229

Comment 26

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 27

[aryan.aftab003]

extra call to load() and set_place() between those two venkman profiles
http://nvpnews.com/india-vs-west-indies-asking-ravindra-jadeja-remained-umpire-injury-to-throw-instantly-sorry-see-video/