348928 - microsummary changes don't trigger bookmarks template rebuild

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Microsummary changes to the bookmarks datasource don't trigger the template builder to rebuild the bookmarks UI (bookmarks toolbar, sidebar, and menu).  The microsummary service employs a hack to force rebuilds when certain arcs get asserted, changed, or unasserted (see bug 348248 for an updated version of the hack), but that just masks the underlying problem.

The bookmarks UI should be getting rebuilt whenever the microsummary service makes changes that affect the presentation of the bookmarks data source via bookmarks templates.

Comment 1

[Neil Deakin]

Does this happen on the trunk as well? Are there some steps I can try, or code to change to see this?
 

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> Does this happen on the trunk as well?

Places doesn't use a XUL template to build the bookmarks toolbar, so the problem isn't applicable there.  I haven't checked to see if Places still uses templates for the bookmarks menu, nor have I tried building without Places on the trunk, but I'll check those out.


> Are there some steps I can try, or code to change to see this?

Yes, it's relatively simple.

Steps to Reproduce:

1. delete the workaround from browser/components/microsummaries/src/nsMicrosummaryService.js.in, which consists of two occurrences of this code block:

    if (fieldName == FIELD_GENERATED_TITLE)
      this._bmds.beginUpdateBatch();

And two occurrences of this code block:

    if (fieldName == FIELD_GENERATED_TITLE)
      this._bmds.endUpdateBatch();


2. rebuild the microsummaries component:

make -C browser/components/microsummaries/


3. start Firefox, go to http://www.woot.com/ , and select Bookmarks -> Bookmark This Page to open the "Add Bookmark" dialog;

4. in the "Name" field, select a live title from the drop-down menu;

5. in the "Create in" field, select the "Bookmarks Toolbar";

6. press the "OK" button to add the bookmark.


Expected Results: the bookmark appears on the toolbar labeled with the live title you selected.

Actual Results: the bookmark appears on the toolbar labeled with the static page title (currently "Woot : One Day, One Deal").


Further Steps to Reproduce:

7. open a new window, noting that the bookmark now appears correctly labeled;

8. open the bookmark properties dialog for the bookmark, and select the static page title from the drop-down menu in the Name field;

9. press the "OK" button to save the change.


Expected Results: the bookmark label changes from the live title to the static title in the bookmarks toolbar.

Actual Results: the bookmark disappears from the bookmarks toolbar.

Comment 3

[Neil Deakin]

The issue is actually caused by changing the rdf type, so the batch updates should be able to be put around those changes rather than the specific field changes. That doesn't fix this bug, but it be more efficient.

I have a reduced testcase which I can post if desired. The bug doesn't occur in the testcase on the trunk (although a possibly related bug does occur), so any fix would be branch only.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> The issue is actually caused by changing the rdf type, so the batch updates
> should be able to be put around those changes rather than the specific field
> changes. That doesn't fix this bug, but it be more efficient.

That doesn't seem to work for me.  When I change the conditional to check for FIELD_RDF_TYPE instead of FIELD_GENERATED_TITLE, the bug reappears (both on setting and on clearing a microsummary).

That's too bad, since if we could switch the workaround to only apply when we set or clear a microsummary, it would mean a signicant performance improvement.  It would also be a usability win, since rebuilding bookmark UI while a user is selecting a bookmark causes Firefox to lose the user's selection.


> I have a reduced testcase which I can post if desired. The bug doesn't occur 
> in the testcase on the trunk (although a possibly related bug does occur), 
> so any fix would be branch only.

Sure, it wouldn't hurt to see the reduced testcase.

Comment 5

[Neil Deakin]

(In reply to comment #4)
> That doesn't seem to work for me.  When I change the conditional to check for
> FIELD_RDF_TYPE instead of FIELD_GENERATED_TITLE, the bug reappears (both on
> setting and on clearing a microsummary).
> 

Did you move the batch calls outside of setField and directly into set/removeMicrosummary ?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: improves workaround by rebuilding on RDF type change

> Did you move the batch calls outside of setField and directly into
> set/removeMicrosummary ?

No, I left them in setField/clearField methods.  Moving them into setMicrosummary/removeMicrosummary works.  Why does that make a difference?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 238074 [details] [diff] [review]
patch v1: improves workaround by rebuilding on RDF type change

Notes for drivers considering this approval request:

This low-risk patch can provide a significant performance improvement when reloading multiple microsummaries.  It's also a usability win, since currently we rebuild all bookmark UI when updating a microsummary, and if a user is selecting a bookmark when that happens, Firefox will lose their selection.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 238074 [details] [diff] [review]
patch v1: improves workaround by rebuilding on RDF type change

I'm going to go out on a limb and review this solo.  I think Neil's on board with the potential solution, but I'd want to get this before tomorrow's nightlies so we can back out, if needed, before freeze.

Comment 9

[Mike Schroepfer]

Comment on attachment 238074 [details] [diff] [review]
patch v1: improves workaround by rebuilding on RDF type change

a=schrep

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Better workaround checked into trunk and branch.  Leaving this bug open until the actual bug gets fixed.

Comment 11

[Neil Deakin]

Myk, yes that change is what I meant.

I can look into the real bug if desired, but it doesn't occur with the new template builder on the trunk, so if the workaround it sufficent, I'm hestitant to fix this only on the branch.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #11)
> Myk, yes that change is what I meant.

Ok, cool.  FWIW, I still don't understand why moving the code makes a difference.  Can you explain why that matters?


> I can look into the real bug if desired, but it doesn't occur with the new
> template builder on the trunk, so if the workaround it sufficent, I'm 
> hestitant to fix this only on the branch.

The workaround seems sufficient, so if it doesn't happen on the trunk, then it's probably not worth further attention.  But we should monitor incoming bugs against Firefox 2 when that gets released; if feedback shows that the workaround is problematic, then we should take another look.

Comment 13

[Neil Deakin]

(In reply to comment #12)
> (In reply to comment #11)
> > Myk, yes that change is what I meant.
> 
> Ok, cool.  FWIW, I still don't understand why moving the code makes a
> difference.  Can you explain why that matters?

Before this patch, you were rebuilding the template after every field was changed. The issue is with changing the RDF type, so you would put the update batch calls around the spot where you change the type. The bug is caused because changing the RDF type would cause a different rule in the template to become active (for example, changing a microsummary to a bookmark), and, for whatever reason, the template builder isn't switching rules properly.

 

Comment 14

[Myk Melez [:myk] [@mykmelez]]

> Before this patch, you were rebuilding the template after every field was
> changed. The issue is with changing the RDF type, so you would put the update
> batch calls around the spot where you change the type.

Ah, indeed.  Sorry, I thought for some reason that we were calling _setField() to change the type, so it didn't make sense to me that the batch calls weren't working in that method.  Now I see that when we change the type, we don't call setField() to do it but rather do it directly from set/removeMicrosummary().

I guess I did it that way because setField() otherwise only sets literal values.  I wonder if it makes more sense to make it support setting resources based on which field is being set.  But in any case that's another bug.

Thanks for your help Neil!

Comment 15

[Myk Melez [:myk] [@mykmelez]]

This doesn't seem worth leaving open.