Closed Bug 524091 Opened 15 years ago Closed 13 years ago

Remove microsummaries support. Seldom used, undiscoverable and unmaintained.

Categories

(Firefox Graveyard :: Microsummaries, defect)

defect
Not set
normal

Tracking

(blocking2.0 -)

VERIFIED FIXED
Firefox 6
Tracking Status
blocking2.0 --- -

People

(Reporter: fryn, Assigned: mak)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [killthem][fixed-in-places])

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

Microsummaries is a feature added with Firefox 2.0; however, the concept did not pick up any traction. It is neither used by the vast majority of users nor easily discoverable.

Evidence for non-use and lack of traction:
http://news.google.com/news?q=microsummaries

Nothing's there!

Reproducible: Always

Steps to Reproduce:
1. Install Firefox.
2. Check if Microsummaries is supported.
Actual Results:  
It is!

Expected Results:  
It shouldn't be.

Microsummaries should be an extension like DOM Inspector or made into a Mozilla Labs project.
Adding [killthem] to the whiteboard so this gets looked at sometime. It's a term we're using to track things we think we might be able to get rid of.
Whiteboard: [killthem]
Status: UNCONFIRMED → NEW
Ever confirmed: true
not sure completely killing microsummaries is worth it, but we could evaluate to reduce their code size and impact, retaining the most useful features of it and pushing only those.
Would adding this to a Text Pilot experiment be helpful in making a decision on this? For starters, we could simply track whether users even touch this feature.
nobody talked about the feature in the last 2 years, so i suspect doing a Test Pilot on it won't bring surprises to the table.
I think the only 2 options are:
- feature removal
- feature refresh/restart

the former is going to remove support for something that some site relies on. Any testing on this would be completely different based on the fact people visits one of those sites or not.
I think makes sense to understand what is appreciated and what not of it, and retain the core experience, dropping any edge case.
Is there a list of various sites that rely on this feature?
A quick Google search only reveals that the few plugins that exist to integrate microsummaries have seen little usage, and blogs haven't covered this feature since 2006.
Give that there 15 known sites to use it, compared to the millions of sites out there, I would say this function is not used. I believe it does add overhead to firefox, and even an advanced user like don't know how to use this.
With apologies for the inventors of this.
I'm unsure how the fact somebody does not use a feature, or does not understand how to use it, can be reasoning to say it is useless.

If i search "microsummary" in Google, i get back about 1 million results, if i search "private browsing" i get 273000 results. is this a valid point? No it is clearly not.

I think the discussion should be about which core parts of this is useful, which is not, restart the feature and sponsorize it to users in the new form.
The number of results does not determine a feature's value at all. My point was that people haven't been talking about this feature since 2006, whereas discussion and promotion of RSS/Atom feeds and even Web slices is more prominent. This is also evidence of lack of discoverability.

What does 'sponsorize' mean? I couldn't find it in a dictionary.
hm, sorry, i suppose you know i'm not english mother tongue. I meant "educate users (and devs) to the feature" or better advertise it.
No worries :)

My understanding is that this feature is designed to allow users an alternative to feeds when they want to receive updates about a particular page. Unfortunately, the vast majority of web pages do not support microsummaries, so it renders this feature almost useless.

Rather than rely on what existing pages have implemented, I think this is a fantastic opportunity to integrate WebChunks: https://addons.mozilla.org/en-US/firefox/addons/versions/8494

Written by Daniel Glazman, WebChunks puts the power in the hands of users. Users pick which portion of the page they want to "watch", and they then receive updates when changes are made to that portion. It also builds on top of existing technologies by implementing most of IE's Web Slices.

Another interesting idea would be to implement something similar to Google Reader's Google-created feeds, which dynamically create feeds for a page that are updated whenever changes are made to the page:
http://googlereader.blogspot.com/2010/01/follow-changes-to-any-website.html
(In reply to comment #11)
> My understanding is that this feature is designed to allow users an alternative
> to feeds when they want to receive updates about a particular page.
> Unfortunately, the vast majority of web pages do not support microsummaries, so
> it renders this feature almost useless.

well actually it is more about providing a bookmark whose title can change dinamically based on information provided by the page (thus the feature is also called livetitles). Suppose for example a bugzilla query bookmark with bugs count changing in title, or stock quotes.

webchunks is an interesting feature, it's not up to me to decide to integrate it or not. but it would clearly require more resources and dev time than "resizing" microsummaries impact.
Google Reader thing is more like an RSS feed creator.
I see. In that case, it would make more sense to rethink microsummaries while reducing its impact.

If the other devs find it worthy, something similar to WebChunks could be integrated with the internal microsummaries support and packaged up as a Mozilla-promoted add-on.
For sure we should try to diet this code piece, it's a 76KB service file, the biggest js component we have in Places, also PlacesUtils is smaller that has a bunch of general purpose stuff...
We should probably kill any generator functionality and just retain websites livetitles support.
Not a blocker.
blocking2.0: ? → -
status2.0: ? → ---
Even when not used, the microsummaries service sets a 15-second timer (granted, it's not doing much): http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsMicrosummaryService.js#59 (MSS_notify)
Version: unspecified → Trunk
Does any other browser support microsummaries?
(In reply to comment #17)
> Does any other browser support microsummaries?

AFAIK, no.
(In reply to comment #4)
> nobody talked about the feature in the last 2 years, so i suspect doing a Test
> Pilot on it won't bring surprises to the table.

I guess you're talking about the unsurprising suspicion that pretty much nobody uses them?

> I think the only 2 options are:
> - feature removal
> - feature refresh/restart
>
> the former is going to remove support for something that some site relies on.

Which site would *rely* on this browser feature? Surely it would only be users that "rely" on this feature? They might go kicking and screaming, but an add-on that periodically updates a static bookmark title might solve their problem.

> Any testing on this would be completely different based on the fact people
> visits one of those sites or not.
> I think makes sense to understand what is appreciated and what not of it, and
> retain the core experience, dropping any edge case.

What is the "core experience" in your opinion? How much more to microsummaries is there than a bookmark with a dynamic title? At that point it seems like it's just a call about whether that's a feature worth keeping or not.
(In reply to comment #20)
> (In reply to comment #4)
> I guess you're talking about the unsurprising suspicion that pretty much nobody
> uses them?

They don't even know we have it. the UI does not make anything to make them really discoverable, articles about it are in obscure wiki pages, no recent blog posts about them. Some websites and weapps use them, but I would not be surprised if users would not know about them till they bookmark one of those pages. So a test pilot is just going to discover what we already know.

> > the former is going to remove support for something that some site relies on.
> 
> Which site would *rely* on this browser feature? Surely it would only be users
> that "rely" on this feature?.

Well, it was presented as something more than a browser feature, something like a suggested web spec that other browser could inherit (it's implemented page side with a <link>). It didn't get enough traction though.

> What is the "core experience" in your opinion? How much more to microsummaries
> is there than a bookmark with a dynamic title? At that point it seems like it's
> just a call about whether that's a feature worth keeping or not.

I completely agree, there should be a small discussion in a moz ng regarding the fact they are considered a useful spec or not by web devs and a final decision by a driver. We have to figure out if we want to completely drop them or just retain the bookmark-with-a-dynamic-title feature (read-only support without any generator thingy).
I wrote a blog article on this removal effort:
http://blog.bonardo.net/2011/04/12/a-proposal-for-dropping-microsummaries
and created a thread in mozilla.dev.apps.firefox for this single removal:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/064147b792f42fa5#
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch Sync changes (obsolete) — Splinter Review
Comment on attachment 526255 [details] [diff] [review]
component files removal

this doesn't need a further review, it's just nsIMicrosummaryService.idl and nsMicrosummaryService.js physical file removal.
Attachment #526255 - Flags: review+
Comment on attachment 526257 [details] [diff] [review]
Sync changes

So, Philipp, I'm not sure if removing the code is enough for Sync or if this could bring issues when the server tries to send a "microsummary" record.type. At first glance looks like the removal should just work.
Attachment #526257 - Flags: review?(philipp)
Attachment #526256 - Flags: review?(mark.finkle)
Comment on attachment 526254 [details] [diff] [review]
browser/dom changes

this includes a dom change in the form of removing the no more useful addMicrosummaryGenerator method from nsISidebar. This will require a SR.
Attachment #526254 - Flags: review?(sdwilsh)
Summary: Remove microsummaries support; neither used nor discoverable → Remove microsummaries support. Seldom used, nor discoverable and unmaintained.
Comment on attachment 526258 [details] [diff] [review]
Places changes

Since this removal also needs some approval, going directly to Dietrich as module owner.
Attachment #526258 - Flags: review?(dietrich)
Summary: Remove microsummaries support. Seldom used, nor discoverable and unmaintained. → Remove microsummaries support. Seldom used, undiscoverable and unmaintained.
Attachment #526256 - Flags: review?(mark.finkle) → review+
Comment on attachment 526257 [details] [diff] [review]
Sync changes

Summarizing the IRC discussion mak, rnewman and I just had:

The patch as it is would drop any incoming microsummaries silently. We should instead just create them as regular bookmarks. This means that once you change them on a newer version of Firefox and sync back to an older one, you might get a bookmark instead of a microsummary. We're ok with that.

We also need a unit test that exercises a new microsummary being synced down, verify it's created as a regular bookmark.
Attachment #526257 - Flags: review?(philipp) → review-
Comment on attachment 526254 [details] [diff] [review]
browser/dom changes

r=sdwilsh
Attachment #526254 - Flags: review?(sdwilsh) → review+
Comment on attachment 526254 [details] [diff] [review]
browser/dom changes

Going to rs for the actual superreview of the nsISidebar method removal.
Attachment #526254 - Flags: superreview?(robert.bugzilla)
Comment on attachment 526254 [details] [diff] [review]
browser/dom changes

I believe you removed all the comments containing addMicrosummaryGenerator else where... r=me with that confirmed.
http://mxr.mozilla.org/mozilla-central/search?string=addMicrosummaryGenerator
Attachment #526254 - Flags: superreview?(robert.bugzilla) → superreview+
(In reply to comment #36)
> I believe you removed all the comments containing addMicrosummaryGenerator else
> where... r=me with that confirmed.
> http://mxr.mozilla.org/mozilla-central/search?string=addMicrosummaryGenerator

yep, I checked those and are removed in the various parts. thanks.
Attached patch Sync changes (obsolete) — Splinter Review
Updated changes to Sync, plus test.
Attachment #526257 - Attachment is obsolete: true
Attachment #527563 - Flags: review?(philipp)
Comment on attachment 526258 [details] [diff] [review]
Places changes


>   _initNamePicker: function EIO_initNamePicker() {
>-    var userEnteredNameField = this._element("userEnteredName");
>     var namePicker = this._element("namePicker");

>+          <textbox id="editBMPanel_namePicker"
>+                   onblur="gEditItemOverlay.onNamePickerChange();"
>+                   observes="paneElementsBroadcaster"/>


r=me. the only improvement would be to remove the term "namepicker" since it doesn't make sense anymore. if you don't do that here, file a followup.
Attachment #526258 - Flags: review?(dietrich) → review+
(In reply to comment #39)
> Comment on attachment 526258 [details] [diff] [review]
> Places changes
> 
> 
> >   _initNamePicker: function EIO_initNamePicker() {
> >-    var userEnteredNameField = this._element("userEnteredName");
> >     var namePicker = this._element("namePicker");
> 
> >+          <textbox id="editBMPanel_namePicker"
> >+                   onblur="gEditItemOverlay.onNamePickerChange();"
> >+                   observes="paneElementsBroadcaster"/>
> 
> 
> r=me. the only improvement would be to remove the term "namepicker" since it
> doesn't make sense anymore. if you don't do that here, file a followup.

editBMPanel_namePicker is likely used by add-ons.
(In reply to comment #40)
> editBMPanel_namePicker is likely used by add-ons.

Yes, I didn't change that for this reason.
The problem is that it was a menulist and now it's a textbox, so I'm not sure if it's better to completely break or only partially break compatibility.
Comment on attachment 527563 [details] [diff] [review]
Sync changes

>-        else
>-          record = new Bookmark(collection, id);
>-        record.title = this._bms.getItemTitle(placeId);
>-      }
>+      else
>+        record = new Bookmark(collection, id);
>+      record.title = this._bms.getItemTitle(placeId);

While you're at it, can you put curly braces around the else block? Thanks!


>+function run_test() {
>+  do_test_pending();

You can get rid of this (see below)

>+  Engines.register(BookmarksEngine);
>+  let engine = Engines.get("bookmarks");
>+  let store = engine._store;
>+
>+  // Clean up after other tests. Only necessary in XULRunner.
>+  store.wipe();
>+  clearBookmarks();

store.wipe() should be sufficient, no? If it's not, that's a bug!

>+  let syncTesting = new SyncTestingInfrastructure(function () {});

You can get rid of this (see below)

>+
>+  initTestLogging("Trace");
>+  Log4Moz.repository.getLogger("Engine.Bookmarks").level = Log4Moz.Level.Trace;
>+
>+  CollectionKeys.generateNewKeys();

and this (see below) (this is purely a test helper and has been moved out of CollectionKeys in services-central already anyway)

>+  _("Convert record to the old microsummaries one.");
>+  record.staticTitle = STATIC_TITLE;
>+  record.generatorUri = GENERATOR_URL;
>+  record.type = "microsummary";
>+  record.prototype = record.__proto__;

I don't understand what this line is supposed to do.

>+  Utils.deferGetSet(record, "cleartext", ["generatorUri", "staticTitle"]);

You're setting staticTitle and generatorUri above already, now you're redefining them as getters and setters? I think you can just get rid of this line.

>+  try {
>+    PlacesUtils.annotations.getItemAnnotation(id, GENERATORURI_ANNO);
>+    do_throw("Bookmark should not have " + GENERATORURI_ANNO + " annotation");
>+  } catch(ex) {
>+    do_check_eq(ex.result, Cr.NS_ERROR_NOT_AVAILABLE);
>+  }
>+  try {
>+    PlacesUtils.annotations.getItemAnnotation(id, STATICTITLE_ANNO);
>+    do_throw("Bookmark should not have " + STATICTITLE_ANNO + " annotation");
>+  } catch(ex) {
>+    do_check_eq(ex.result, Cr.NS_ERROR_NOT_AVAILABLE);
>+  }

Could use do_check_throws here. It doesn't exist in Sync's head* files, so you'd have to steal it from somewhere. But I want to add it to the test harness anyway (bug 650402).


From here...

>+  _("Sync record to the server.");
>+  Svc.Prefs.set("username", "foo");
>+  Svc.Prefs.set("serverURL", "http://localhost:8080/");
>+  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
>+
>+  let collection = new ServerCollection({}, true);
>+  let global = new ServerWBO('global',
>+                             {engines: {bookmarks: {version: engine.version,
>+                                                    syncID: engine.syncID}}});
>+  let server = httpd_setup({
>+    "/1.1/foo/storage/meta/global": global.handler(),
>+    "/1.1/foo/storage/bookmarks": collection.handler()
>+  });
>+
>+  try {
>+    engine.sync();
>+    let wbos = [id for ([id, wbo] in Iterator(collection.wbos))
>+                   if (["menu", "toolbar", "mobile"].indexOf(id) == -1)];
>+    do_check_eq(wbos.length, 1);
>+
>+    _("Verify that the server WBO does not have the annotation.");
>+    let serverGUID = wbos[0];
>+    do_check_eq(serverGUID, guid);
>+    let serverWBO = collection.wbos[serverGUID];
>+    do_check_true(!!serverWBO);
>+    let body = JSON.parse(JSON.parse(serverWBO.payload).ciphertext);
>+    do_check_false("staticTitle" in body);
>+    do_check_false("generatorUri" in body);
>+  } finally {
>+    // Clean up.
>+    store.wipe();
>+    Svc.Prefs.resetBranch("");
>+    Records.clearCache();
>+    clearBookmarks();
>+    server.stop(do_test_finished);
>+  }
>+}

... to here isn't really necessary, you're already unit-testing the necessary bits (createRecord and applyIncoming). So you can get rid of it, as well as some of the helpers (do_test_pending, SyncTestingInfrastructure, generateKeys).

r=me with that
Attachment #527563 - Flags: review?(philipp) → review+
do I have to call store.wipe(); and Records.clearCache() at the end of the test still? usually xpcshell tests are independent but I see you have comments about cleanup needed for xulrunner(?)
(In reply to comment #43)
> do I have to call store.wipe(); and Records.clearCache() at the end of the test
> still?

Records.clearCache(): no
store.wipe(): maybe? In the long run I want Sync to do what Places does here which I believe is bug 620473

> usually xpcshell tests are independent but I see you have comments about
> cleanup needed for xulrunner(?)

xulrunner was the xpcshell alternative harness we have for the 3.x add-on code.
(In reply to comment #44)
> xulrunner was the xpcshell alternative harness we have for the 3.x add-on code.
What philikon means by this is no, you don't have to care about it because they no longer support the 3.x code.
Attached patch Sync changes (obsolete) — Splinter Review
updated based on comments, added do_test_throws() to head_helpers.js, can be easily ported to the harness (with a dedicated harness test).
Attachment #527563 - Attachment is obsolete: true
Attached patch Sync changesSplinter Review
qrefreshed, too :(
Attachment #527931 - Attachment is obsolete: true
dev-doc-needed for the removal of microsummaries related docs from MDN (I think there isn't much, most of this stuff was still in the wiki).

Need to figure out the SUMO side before merging back to central.
Keywords: dev-doc-needed
(In reply to comment #49)
> Need to figure out the SUMO side before merging back to central.
We only really need SUMO figured out for the merge to aurora, right?
(In reply to comment #50)
> (In reply to comment #49)
> > Need to figure out the SUMO side before merging back to central.
> We only really need SUMO figured out for the merge to aurora, right?

In services meeting today, we talked about how this change can affect what Sync work is doing concurrently in services-central branch.   Can we have a discussion on this work either checked into s-c, or merged into m-c so s-c branch can pick up the changes?    

The purpose here is so QA can test sync regression scenarios in s-c before it's merged to m-c.

Philikon can chime in more.
(In reply to comment #50)
> (In reply to comment #49)
> > Need to figure out the SUMO side before merging back to central.
> We only really need SUMO figured out for the merge to aurora, right?

Ideally, but it's worth investigating the needs before going to central.

(In reply to comment #51)
> In services meeting today, we talked about how this change can affect what Sync
> work is doing concurrently in services-central branch.   Can we have a
> discussion on this work either checked into s-c, or merged into m-c so s-c
> branch can pick up the changes?    

I think we could merge Places to s-c without any big trouble, so far Places only contains mozilla-central (updated to this morning) and the microsummaries removal.
I've sent a mail to Tenser to have a guess on SUMO needs.
(In reply to comment #52)
> I think we could merge Places to s-c without any big trouble, so far Places
> only contains mozilla-central (updated to this morning) and the microsummaries
> removal.

Note that s-c is on a weekly merge train heading towards m-c. That would mean if we merged places to s-c, your changes would make it to m-c the following Tuesday. If you want that, great. If you don't, then I suggest you merge to m-c at your own leisure (but preferably soon-ish).
ok, then my plan is to merge to central today, I'm still waiting answers on SUMO but at this point it's better to give this change to testers earlier and fix surrounding stuff before going to Aurora.
No problems on SUMO side, David just mailed me that this was not deeply documented from the beginning, as such the removal has practically no impact on SUMO.
http://hg.mozilla.org/mozilla-central/rev/914e10b323c1
http://hg.mozilla.org/mozilla-central/rev/034a23531362
http://hg.mozilla.org/mozilla-central/rev/5e98268edaf1
http://hg.mozilla.org/mozilla-central/rev/512f90d0dce1
http://hg.mozilla.org/mozilla-central/rev/a6ef6163855d

If you need any assistance related to this change please let me know by mail or ping me on IRC.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Notes for testing:
- test existing microsummaries will now become a fixed title on a fixed build
- test creating mircosummaries UI is disabled
- create profile on old build with bookmarks w/ microsummaries, then sync.  Launch profile on fixed build, and then sync.  Verify bookmarks w/ microsummaries show fixed titles
Depends on: 654292
Did this kill live bookmarks, too? People on Mozillazine are saying live bookmarks no longer work.
Live bookmarks seems to updating for me, they are not totally broken, but the context menu on right-click is missing.  Using the Firefox/App button->Bookmarks->Livemarks the 'reload function is missing, as well as from the menu-bar. 

Confirmed missing on Win32 m-c latest hourly. If you use the 'bookmarks' button the 'reload function' is there on right-click, as well if you open your bookmarks in the sidebar with ctrl+b. It appears in the Library as well.

Anyone know if this was 'intentional' ? or perhaps a side-affect of the removal of Micro-summaries.
The missing context item for 'reload Live Bookmark' - is also missing from 4.0.1, so the removal of micro-summaries is likely not the cause.  Perhaps now, what we need is a new bug, or confirmation that dropping it from certain areas of various context-menus is intentional, or an oversight.
(In reply to comment #60)
> Did this kill live bookmarks, too? People on Mozillazine are saying live
> bookmarks no longer work.

no, this code is completely separate from live bookmarks. You have to file a new bug, a real regression range would be nice too.
Also, it's not possible to have a difference in live bookmarks between 4.0 and 4.0.1.
Has anybody noticed this change renders editing of bookmark names impossible?

I cannot rename my bookmarks at all!
(In reply to comment #64)
> Has anybody noticed this change renders editing of bookmark names impossible?
> 
> I cannot rename my bookmarks at all!

works fine here, maybe you have an add-on creating issues. Try contacting support at http://support.mozilla.com/ please.
(In reply to comment #11)
> Rather than rely on what existing pages have implemented, I think this is a
> fantastic opportunity to integrate WebChunks:
> https://addons.mozilla.org/en-US/firefox/addons/versions/8494
Could such an integration find http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#using-the-microdata-dom-api useful once bug 591467 implements it? Perhaps it could also make semantic use of <summary> elements? (bug 591737)
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue using FF 6.0b3 on Windows XP, Windows 7, Ubuntu  and Mac OS X 10.6.
Microsummaries are no longer supported (used some of the sites from the wiki page from Comment 6 to verify this https://wiki.mozilla.org/Microsummaries/Sites)

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Farewell, "Live Titles"
Neat Woot, Hanselman bookmarks
But you were just cruft
Blocks: 745410
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: