592436 - Allow badging items in the awesomebar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We'd like to add a feature where code can provide an informational "badge" to items in the awesomescreen results. An example could be adding the number of unread inbox messages for Gmail. Or how many unread tweets exist since the last time you looked at Twitter.

Not all items would have badges, only those supported by built-in code or add-ons. We might want an in-memory map of URLs and badge text. Something that would be fast and lightweight.

Another possible approach could be to use Annotations, but that could be a performance hit.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This is a simple backend using a in-memory map as a glue between the awesome bar and the "badges".
The update is called once the search is completed with a delay of 300ms to avoid unuseful results between keypress. 

There is also a css transition when the badge appear (it become more and more opaque), otherwise there was a feeling of popping up.

The handler registered for a URL receive the xul element as the first argument so it could show whatever he want quickly before doing a an asynchronous operation if he wants to. 
I'm not sure we should pass the xul element directly instead of a wrapper to avoid setting the badge on the wrong element if it moves away for any reason but this can be done in a followup if needed.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 471531 [details] [diff] [review]
Patch

>diff -r f482950a917d chrome/content/bindings.xml

>+          <xul:label class="autocomplete-item-badges" value="" xbl:inherits="value=badges"/>

We are only adding a single badge to each row, right? I think "badge" is better than "badges"
Change all *badges* to *badge*, except where you are actually holding an array of badges

>   <binding id="place-item" extends="chrome://browser/content/bindings.xml#place-base">

>         </xul:vbox>
>+          <xul:label class="bookmark-item-tags" xbl:inherits="value=tags"/>
>+        <xul:vbox>
>+        </xul:vbox>
>       </xul:hbox>

Why do we have an empty vbox here? Do we need it for spacing? If so couldn't CSS handle the spacing?

>diff -r f482950a917d chrome/content/browser-ui.js

>+var PageBadges = {

I wonder if this code shouldn't be in the autocomplete popup binding so it is part of the binding itself. I suppose it is more general purpose in browser-ui, but do we want it to be general purpose?

>+  add: function pb_add(aURL, aCallback) {
>+    let effectiveHost = this._getEffectiveHost(Services.io.newURI(aURL, null, null));
>+    this._badges[effectiveHost] = aCallback;

I would return if aCallback is null

>+  remove: function pb_remove(aURL) {

I like "registerHandler" and "unregisterHandler"

>+  assignTo: function pb_assignTo(aItems) {

"updateBadges"

>+    this._timeout = window.setTimeout(function(self) {
>+      for (let i = 0; i < aItems.length; i++) {
>+        let item = aItems[i];
>+        if (!item.hasAttribute("url"))
>+          return;

Do you want to "return" here? Maybe you should just "continue" and skip this item

>+        let itemHost = self._getEffectiveHost(Services.io.newURI(item.getAttribute("url"), null, null));
>+        for (let badgeHost in self._badges) {
>+          if (itemHost == badgeHost && self._badges[badgeHost]) {

You probably don't need the "&& self._badges[badgeHost]" check here

>+            self._badges[badgeHost](item);

We should also support objects, not just functions. If the callback is an object and has "updateBadge", we could call that function, passing the item as a param?

>diff -r f482950a917d chrome/content/browser.xul

>+                             onsearchcomplete="PageBadges.assignTo(this.popup._items.childNodes);"

I'm not sure if we need this, if we move the code into the binding

Overall this looks good. Before I r+ though, I'd like your thoughts on moving the code to the binding.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Addressed comments.

After have thoughts more on it, I think moving it to the bindings is the right stuff to do it now since there is no obvious use for it in the core code and extensions can still access the _badges field.

Comment 4

[Aakash Desai [:aakashd]]

This is a part of the awesome screen redesign. I'm cc'ing Aaron to this bug.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 472369 [details] [diff] [review]
Patch v0.2

>diff -r aeb8aa5a377f chrome/content/bindings.xml

>+      <method name="registerHandler">

>+      <method name="unregisterHandler">

Since we moved this to the binding, let's be more explicit: registerBadgeHandler, unregisterBadgeHandler

Nice patch. Can't wait to try it out.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/e35ea631ac53

Comment 7

[Tony Chung [:tchung]]

Verified fix on

Mozilla/5.0 (Maemo; Linux armv7l; en-US; rv:2.0b6pre) Gecko/201000913 Firefox/4.0b6pre Fennec/2.0b1pre

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b5pre) Gecko/20100910 Firefox/4.0b6pre Fennec/2.0b1pre

Comment 8

[Aaron Train [:aaronmt]]

https://litmus.mozilla.org/show_test.cgi?id=12892