337868 - Implement result view for toolbar for dynamic updating

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brett Wilson]

The new HistoryResultView API allows the toolbar to be dynamically changed. Currently, the entire toolbar is rebuilt when anything on the toolbar changes, which can be really slow.

Bug 337855 is doing the same thing for the bookmarks menu.

Comment 1

[Joey Minta]

Attachment: wip-patch

This is a work in progress patch for incrementally updating the toolbar.  I'd like to get some feedback on whether I'm on the right track before digging in to fix the trickier bits.

Note: currently onstartupdatebatch/onendupdatebatch is still getting called way too often, meaning that the changes here won't yet make much difference, but commenting out those functions will get you basic incremental updating.

Comment 2

[Neil Deakin]

jminta, is this patch still being worked on? What still needs to be fixed?

Comment 3

[Joey Minta]

Neil,
    The patch doesn't actually implement the result-view interface.  Some of the changes to the internal function would probably be useful in doing that though.  At this point my schedule looks pretty crappy for the next month, so I don't see getting to this in the near future.  Anyone should feel free to jump in here.

Comment 4

[Neil Deakin]

Attachment: implement toolbar updating

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> Created an attachment (id=262682) [details]
> implement toolbar updating
> 

hi neil, is this a wip patch, or ready for review?

Comment 6

[Neil Deakin]

It can be reviewed, but I based it on your menu updating patch in bug 337855 which you asked mano about confirming the approach, so I had planned on waiting until then.
 

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> It can be reviewed, but I based it on your menu updating patch in bug 337855
> which you asked mano about confirming the approach, so I had planned on waiting
> until then.
> 
> 

ok, thanks neil. mano said this approach is fine. can you ask him to review this, as he's spent much time in this code.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 262682 [details] [diff] [review]
implement toolbar updating

Nice, r=mano. Please file a bug on optimizing itemChanged though, it shouldn't need to re-create the DOM node.

Comment 9

[tabmix.onemen]

Warning: reference to undefined property this._anno
Source file: chrome://browser/content/places/toolbar.xml
Line: 222

-  this._anno.getAnnotationString(containerURI, "livemark/siteURI");
+  PlacesUtils._annotations.getAnnotationString(containerURI, "livemark/siteURI");

Comment 10

[tabmix.onemen]

after this bug landed _viewer.itemChanged only called after the tool bar button clicked (i.e. the container opened) once

to test this try to change the title before you click the bookmark button on the toolbar > the title on the toolbar don't changed.
now click on the bookmark button once and try again to change the title.

Comment 11

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

> to test this try to change the title before you click the bookmark button on
> the toolbar the title on the toolbar don't changed.

onemen.one, can you clarify the steps to reproduce this problem?  i'm having problems reproducing what you are describing.

Comment 12

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

> Please file a bug on optimizing itemChanged though, it shouldn't
> need to re-create the DOM node.

neil logged bug #379591

Comment 13

[tabmix.onemen]

(In reply to comment #11)
> > to test this try to change the title before you click the bookmark button on
> > the toolbar the title on the toolbar don't changed.
> 
> onemen.one, can you clarify the steps to reproduce this problem?  i'm having
> problems reproducing what you are describing.
> 

1. open Firefox.
2. make sure your Bookmarks Toolbar is visible.
3. open Bookmarks Manager.
4. in Bookmarks Manager change the title for folder or livemark item.

result:
the item title changed in the Bookmarks Manager but didn't changed in the Bookmarks Toolbar.

if you do step 4 after you'v open the folder from Bookmarks Toolbar its work well.

Comment 14

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

onemen.one, thanks for the steps.

i was trying this with bookmark items, and not folders or livemarks.  now I understand and I am able to reproduce this issue.

I've spun this issue off to bug #380205.

Comment 15

[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