334471 - (microsummaries) implement support for microsummaries via a service and UI integration

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

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

Firefox should support microsummaries via a service that keeps them up-to-date and UI integration into the bookmark properties dialog and bookmarks toolbar.

Comment 1

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

Attachment: patch v1: an initial implementation of microsummary support

Here's an initial implementation of support for microsummaries.  It includes the features in the walkthrough, except that microsummary definition installation now works like search engines, and feed microsummaries aren't supported yet (but the architecture for including them is mostly in place).

Comment 2

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

Attachment: patch v2: like v1, but with sample microsummary definitions

The first patch I attached didn't include the bundled microsummary definitions or the directory they live in (browser/locales/en-US/microsummary-definitions/).  I don't want to add that directory to the repository yet, in case it changes during review and revision, so I used cvsdo to include it in this patch without actually adding it to the repository.

To test installation of microsummary definitions, go to:

http://www.melez.com/mozilla/microsummaries/definitions.html.

Comment 3

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

Comment on attachment 218877 [details] [diff] [review]
patch v2: like v1, but with sample microsummary definitions

It looks like the Firefox product doesn't support setting multiple review requests, so I'll instead request review from Brett and super-review from Ben.

Comment 4

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

Attachment: patch v2 for trunk

Bug 315346 moved nsISidebar.idl from browser/components/sidebar/public/ to dom/public/idl/sidebar/.  Seems like that should be checked into the branch, too, given that browser/ is supposed to be synced between trunk and branch, so I requested approval-branch-1.8.1 on that bug.

In the meantime, here's a patch that works on the trunk (the previous patch was generated on the branch).

Comment 5

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

Comment on attachment 218877 [details] [diff] [review]
patch v2: like v1, but with sample microsummary definitions

I've noticed a couple problems with this patch.  I'll post a new patch soon.

Comment 6

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

Attachment: patch v3 for branch: transactions, more robust async model

This patch integrates microsummary changes into bookmark change transactions and employs a more robust model for asynchronous loading of microsummaries into the bookmark properties dialog.  I also factored out some code for loading and parsing content.  To test the feature, follow the test cases described on this wiki page:

http://wiki.mozilla.org/Microsummaries:Testing

Comment 7

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

Attachment: patch v3 for trunk: transactions, more robust async model

This is the trunk version of patch v3, which integrates microsummary changes into bookmark change transactions and employs a more robust model for asynchronous loading of microsummaries into the bookmark properties dialog.  I also factored out some code for loading and parsing content.

To test the feature, follow the test cases described on this wiki page:

http://wiki.mozilla.org/Microsummaries:Testing

Comment 8

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

Comment on attachment 219607 [details] [diff] [review]
patch v3 for branch: transactions, more robust async model

This needs to be updated to work with the old bookmarks code, but in the meantime it could still benefit from review of the overall architecture, code style, UI model, etc.

Comment 9

[Brett Wilson]

I didn't check in too much detail...

addDefinition: URIs are almost never represented as UTF16, which your function takes. I think this shuold either be UTF8 or a nsIURI.

Some search and replace got out of control "secpageURIty"

I think sanitizeName when it generantes random names access the array off by one. You should multiply by the length - 1.

This sets some annotations on the page. For the old history service, anything will need to be per-bookmark. There is reasonable chance places will be changed to work like this too, so you might want to change how the service does this.

Your frame loading/XMLHTTP thing confused me, but I assume you talked to the right people who understand this stuff.

Comment 10

[Brett Wilson]

Can you explain the changes to toolbar.xml? Maybe you should check with Annie.

I think addDefinition should take a nsIURI instead of a string.

Comment 11

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

Attachment: work in progress

Here's a work in progress.  I'm still ironing out some kinks, and I need to clean it up a bit, but it's largely there, and it's been retooled to work with both the old bookmarks code and the new Places code.


>addDefinition: URIs are almost never represented as UTF16, which your function
>takes. I think this shuold either be UTF8 or a nsIURI.

Makes sense.  I'll change it to an nsIURI.  I've been using a string because that's how the search engine code that I was emulating does it, but there's no reason the microsummaries code couldn't do it differently.


>Some search and replace got out of control "secpageURIty"

Thanks for the catch; I'll fix that.


>I think sanitizeName when it generantes random names access the array off by
>one. You should multiply by the length - 1.

Yeah, you're right.  I'll fix that here and also in the search service code from which I copied this function.


>This sets some annotations on the page. For the old history service, anything
>will need to be per-bookmark. There is reasonable chance places will be changed
>to work like this too, so you might want to change how the service does this.

Good point.  The patch I'm working on now takes this approach, identifying bookmarks by bookmark ID rather than page URI.

In the Places version, I still use the page URI as the bookmark ID, since that's how bookmarks are identified in the annotations datastore at this point.  But I nevertheless distinguish between page URIs used as IDs and those used as actual page URIs (i.e. I don't assume "bookmarkID" variables will contain the page URI and obtain the URI separately when I actually need it).

Thus the code will be easier to update if/when we change the ID model in the new code.


>Your frame loading/XMLHTTP thing confused me, but I assume you talked to the
>right people who understand this stuff.

According to bz, hidden iframes is currently the best way to load and parse HTML pages, which we need to do to process them against the XSLT stylesheet, and I'm following his and others' recommendations to disable images and JavaScript in the iframe to minimize load and prevent the page from doing things with JavaScript that the user might notice (and thus wouldn't be so hidden).


>Can you explain the changes to toolbar.xml? Maybe you should check with Annie.

The code I'm changing in toolbar.xml is all code I added with my patch in bug 333052.  Now that I'm actually exercising that code, I've noticed two issues with it:

1. When a bookmark gets added to the bookmarks toolbar, it doesn't get added to the list of displayed bookmarks (currentURIs) until the toolbar gets rebuilt.

Rebuilding happens almost right away, of course, but in the intervening microseconds the annotation observer gets called to update the cache of generated titles to take the new bookmark into account.

And since the bookmark isn't in the list of displayed bookmarks yet, the annotation observer doesn't update the cache, and thus the new bookmark's generated title doesn't get displayed.

To fix this problem, I have the "add bookmark" observer add the bookmark to the list of displayed bookmarks immediately rather than waiting for the rebuild to do the job.

2. The annotation observer only updates the cache of generated titles for displayed bookmarks.

But when we first build the cache, we actually store all generated titles, not just those for displayed bookmarks, for simplicity and because we generate the cache before we have a list of all displayed bookmarks.

So when the observer selectively updates the cache, the cache gets out of sync with the state of the generated titles in the annotations datastore.  And if a bookmark with a generated title gets added to the toolbar, and its generated title was updated since the toolbar was instantiated, the toolbar will show a stale generated title rather than the latest one.

To fix this problem, I update the cache when any generated title changes, not just when a displayed bookmark's generated title changes.

Comment 12

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

>The code I'm changing in toolbar.xml is all code I added with my patch in bug
>333052.

Err, to be precise, the bookmark observer changes aren't changes to code I added in that patch, since the bookmark observer wasn't added in that patch, but the changes themselves are to make the observer touch the currentURIs property, which is a property I added in that patch.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> > addDefinition: URIs are almost never represented as UTF16, which your
> > function takes. I think this shuold either be UTF8 or a nsIURI.
> 
> Makes sense.  I'll change it to an nsIURI.  I've been using a string because
> that's how the search engine code that I was emulating does it, but there's no
> reason the microsummaries code couldn't do it differently.

I chose to take a string for addEngine instead of an nsIURI because most callers had no nsIURIs handy, and putting the burden on them to create one seemed unnecessary. I don't know whether that will be the case for microsummaries - from looking at the patch it looks like there is only one caller at the moment, and it doesn't have a URI object. I suppose I could have used AUTF8String instead of AString, but I suspect it doesn't make much of a difference for interfaces that are used by and implemented in JS (in fact, I think it might introduce an unnecessary conversion across XPConnect).

Comment 14

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

Attachment: patch v4 for branch: fixes review issues; works with old bookmarks code

This patch includes fixes for all the issues discussed in comment 11 as well as:

1. changes "definitions" to "generators" to covey their meaning better;
2. supports both Places and the old RDF-based bookmarks service;
3. miscellaneous other improvements, code cleanup, refactoring, and documentation;
4. no longer includes sample generators; we shouldn't ship those until we can have discussions with the sites/partners involved, but in the meantime you can get them at:

http://www.melez.com/mozilla/microsummaries/generators/

And you can test microsummaries functionality by going through these test cases:

http://www.melez.com/mozilla/microsummaries/generators/

Comment 15

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

Attachment: patch v5: synced trunk/branch; works around bug 336194

Now that I've checked in the fix for bug 315346 on the branch, there are no longer any conflicts that prevent a single microsummaries patch from applying cleanly to both the trunk and the branch, so this patch works on both.

I also added a workaround for bug 336194 so the microsummary service and bookmarks toolbar work properly on profiles without any microsummary bookmarks.

Comment 16

[Brett Wilson]

Comment on attachment 220487 [details] [diff] [review]
patch v5: synced trunk/branch; works around bug 336194

Backend looks OK.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220487 [details] [diff] [review]
patch v5: synced trunk/branch; works around bug 336194

I will conduct a full sr of this code once I am not so doomed with rss stuff. I will follow up on this separately after the a2 deadline. In the interim a=[e-mail] for the branch

Comment 18

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

Attachment: patch v6: works around bug 336601 horkage

Wednesday's checkin for bug 319263 horked certain uses of the array methods "some" and "filter", which the microsummaries patch uses.  The horkage is being fixed in bug 336601, but it's unclear when that's going to land, so in the meantime here's a patch that works around the horkage by using other techniques to accomplish the same ends.

This patch also includes updates to packages-static.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220988 [details] [diff] [review]
patch v6: works around bug 336601 horkage

I reviewed the workaround changes... r=brettw for the original patch, r=ben for the workaround changes a=ben for the branch

Comment 20

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

Patch checked in to trunk and branch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

For what it's worth, this looks like it might be the checkin that caused the Rlk regression that the trunk is closed for...  Here's hoping bug 336922 fixes that.

Comment 22

[Darin Fisher]

>Index: dom/public/idl/sidebar/nsISidebar.idl
>===================================================================
>RCS file: >/cvsroot/mozilla/dom/public/idl/sidebar/nsISidebar.idl,v
>retrieving revision 1.3.10.2
>diff -u -r1.3.10.2 nsISidebar.idl
>--- dom/public/idl/sidebar/nsISidebar.idl	2 May >2006 05:59:51 -0000	1.3.10.2
>+++ dom/public/idl/sidebar/nsISidebar.idl	5 May >2006 21:36:11 -0000
>@@ -53,6 +53,7 @@
>                             in string aCustomizeURL);
>     void addSearchEngine(in string engineURL, in >string iconURL,
>                          in wstring suggestedTitle, in >wstring suggestedCategory);
>+    void addMicrosummaryGenerator(in string >generatorURL);
> };

So, this appears to have changed the nsISidebar interface without changing the UUID of the interface.  That means that it is impossible to determine from a C++ extension if the addMicrosummaryGenerator function exists (w/o resorting to checking the version of the app at runtime).  Was this change reviewed by a DOM peer?

Comment 23

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

> So, this appears to have changed the nsISidebar interface without changing 
> the UUID of the interface.  That means that it is impossible to determine 
> from a C++ extension if the addMicrosummaryGenerator function exists (w/o
> resorting to checking the version of the app at runtime).  Was this change
> reviewed by a DOM peer?

No, it wasn't, which is presumably how this mistake managed to make its way into the codebase).  My apologies for that; I should have requested review of this portion from a DOM peer.  How should the code problem best be addressed going forward?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, man.  Several issues here:

1)  On trunk, just change the IID, I guess.
2)  On branch, you can't change this interface.  You need a different
    solution and that needs to block whatever release this gets shipped in, imo,
    since we promised API stability on the branch.  God help us if we've already
    shipped 1.8a2.  :(
3)  You changed the interface, but didn't fix the existing implementation in
    xpfe (which could probably just throw from the method or something).  At
    least file a bug on the Seamonkey folks to do this.
4)  The new method has absolutely no documentation.  I know that's what the
    interface already looks like, but we're moving away from that sort of thing,
    last I checked, as far as humanly possible.

I'm not going to say what I think of the "review" system for Firefox....

Comment 25

[[e-mail]]

(In reply to comment #22)
>So, this appears to have changed the nsISidebar interface without changing the
>UUID of the interface.  That means that it is impossible to determine from a
>C++ extension if the addMicrosummaryGenerator function exists
Worse, it's impossible to tell whether the function works :-(