Last Comment Bug 420605 - fragment links have wrong favicon and description in history entry
: fragment links have wrong favicon and description in history entry
Status: RESOLVED FIXED
: polish
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla7
Assigned To: Justin Lebar (not reading bugmail)
:
: Andrew Overholt [:overholt]
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on: 385434 503832
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-02 12:23 PST by Ori Avtalion (salty-horse)
Modified: 2011-05-25 10:13 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proof of concept (5.93 KB, application/x-xpinstall)
2008-06-03 03:08 PDT, John P Baker
no flags Details
screenshot with and without extension installed (40.99 KB, image/jpeg)
2008-06-04 02:37 PDT, John P Baker
no flags Details
Proof of concept v0.2 (6.48 KB, application/x-xpinstall)
2008-06-12 01:46 PDT, John P Baker
no flags Details
WiP (3.34 KB, patch)
2009-03-17 02:01 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v1 (5.47 KB, patch)
2009-07-06 03:59 PDT, John P Baker
no flags Details | Diff | Splinter Review
testcase (331 bytes, text/html)
2009-07-07 06:25 PDT, John P Baker
no flags Details
failure (103 bytes, text/html)
2009-07-08 03:59 PDT, John P Baker
no flags Details
failure, slightly modified (234 bytes, text/html)
2009-07-08 09:46 PDT, Justin Lebar (not reading bugmail)
no flags Details
patch v2a (favicon) (5.09 KB, patch)
2009-07-10 05:36 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v3 (2.65 KB, patch)
2009-07-13 01:37 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v4 (2.61 KB, patch)
2009-07-17 05:40 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v5 (2.39 KB, patch)
2009-07-17 07:25 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v5.1 (2.47 KB, patch)
2009-07-23 05:38 PDT, John P Baker
no flags Details | Diff | Splinter Review
patch v5.2 (2.30 KB, patch)
2009-07-29 01:08 PDT, John P Baker
no flags Details | Diff | Splinter Review
Test case and bugfix based on John's v5.2 patch (9.59 KB, patch)
2010-03-11 11:04 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review
Test case and bugfix, updated for bug 544097 (9.59 KB, patch)
2010-03-18 07:57 PDT, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review
Patch v1 - Send notification from docshell (13.42 KB, patch)
2011-05-22 09:58 PDT, Justin Lebar (not reading bugmail)
jonas: review+
gavin.sharp: feedback+
Details | Diff | Splinter Review

Description Ori Avtalion (salty-horse) 2008-03-02 12:23:22 PST
Using today's nightly build.

When navigating to a page fragment, a new history item is created.
Instead of having the page's favicon, it has the default favicon.
Instead of having the page's title for a description, it has the filename part of the URL.

To reproduce:
1) Visit http://en.wikipedia.org/wiki/Mozilla_Firefox
2) Click on "Release history" in the table of contents.
*) Now at http://en.wikipedia.org/wiki/Mozilla_Firefox#Release_history
3) Open the history sidebar, sort by last visited.
*) The last entry is Mozilla_Firefox, and it has a default favicon, instead of "Mozilla Firefox" and the Wikipedia favicon.

Note that the back/forward history menu displays the correct description for the fragment.
Comment 1 Ria Klaassen (not reading all bugmail) 2008-03-03 10:58:06 PST
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030307 Minefield/3.0b4pre

I see this too. I see this at least since end May and branch has no icons at all in history so it is not a regression.
Comment 2 Ria Klaassen (not reading all bugmail) 2008-03-03 11:01:24 PST
(In reply to comment #1)
> branch has no icons at
> all in history 

I meant only default favicons.

Comment 3 Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m) 2008-05-25 01:43:56 PDT
Currently I see there's two history entries after reproducing steps: one for "Mozilla Firefox - Wikipedia etc" with Wikipedia icon, and one for Mozilla_Firefox#Release_history, named Mozilla_Firefox, without favicon. Also in back/forward menu Mozilla_Firefox#Release_history has no favicon. 

IMO anchor links must not only have favicons, but they should be distinguished adding them anchor link name in history (in this example, Mozilla_Firefox#Release_history instead of Mozilla_Firefox).

Anyway, confirmed.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052406 Minefield/3.0pre
Comment 4 John P Baker 2008-06-03 03:08:55 PDT
Created attachment 323539 [details]
Proof of concept

This is a proof of concept for the icons; It basically works by shadowing the fragment entry in places with an entry for the page. This seems to work pretty well although it may still need a visit entry to keep the place entry alive for longer.

The second part is The first part is a proof of concept for a problem that history has in displaying a suitable icon on visiting a fragment of a document - three guesses as to the bug number.

The second part is  just some early ideas for the titles but, as it already seems to work quite well, is included for further experimentation.

I wasn't that keen on just showing #fragment but then it occurred to me that I could attempt to use information from the document, for example:

Bug 420605 – fragment links have wrong favicon and description in history entry [Description]
All-in-One Sidebar :: Firefox Extras [Avaliações]
Web Server Statistics for Central Web Server [Redirection Report]

and just use #fragment as a fallback

Bristol University | A-Z index | Full [#w]

There may be various problems with this - it doesn't seem to handle javascript changes very well - so the option is off by default.
Comment 5 John P Baker 2008-06-03 05:09:46 PDT
(In reply to comment #4)
> The second part is The first part is a proof of concept for a problem that
> history has in displaying a suitable icon on visiting a fragment of a document
> - three guesses as to the bug number.

Sorry, I don't know how that 'paragraph' got back in.
Comment 6 John P Baker 2008-06-04 02:37:02 PDT
Created attachment 323688 [details]
screenshot with and without extension installed
Comment 7 Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m) 2008-06-04 04:38:39 PDT
This is an interesting workaround, but you think you can implement it in Firefox C++ source code?
Comment 8 John P Baker 2008-06-12 01:46:02 PDT
Created attachment 324751 [details]
Proof of concept v0.2

(In reply to comment #4)
> There may be various problems with this - it doesn't seem to handle javascript
> changes very well - so the option is off by default.

This turned out to be bug 438288
Comment 9 John P Baker 2009-03-17 02:01:37 PDT
Created attachment 367740 [details] [diff] [review]
WiP

I have been sitting on this for a while so as to see the fate of the HTML5 (IE8) hashchange event (bug 385434); My preferred solution is to try an event listener for this (proposed) event and to set the icon and title in the database to the current ones.

In the meantime the first part of this patch catches the hash change, in the onLocationChange of the progress listener, by comparing any Location containing a '#' with the previous location; The second part is what the event handler would do if it was available.

For nightly testers, I will upload a new extension containing this code (version 0.4) to amo later today.
Comment 10 John P Baker 2009-07-06 03:59:46 PDT
Created attachment 386967 [details] [diff] [review]
patch v1

hashchange has now been implemented, so possibly something like this.

I don't believe that I need the try block anymore but it is useful for testing
Comment 11 John P Baker 2009-07-07 00:39:45 PDT
The extension has been updated (0.5) on amo to reflect this latest change.

[Nit^H^Hote for self: despite other handlers using 'evt' or 'event', change parameter name to 'aEvent']
Comment 12 John P Baker 2009-07-07 06:25:03 PDT
Created attachment 387185 [details]
testcase
Comment 13 Dietrich Ayala (:dietrich) 2009-07-07 10:32:42 PDT
john, can you ask review from gavin or another browser peer for this?
Comment 14 John P Baker 2009-07-08 03:59:11 PDT
Created attachment 387422 [details]
failure

Hmm! I don't get a hashchange for this case but I do get a history entry; I am not sure if it is valid, but unraveling the specs is proving a bit hard...
Comment 15 Justin Lebar (not reading bugmail) 2009-07-08 09:46:28 PDT
Created attachment 387468 [details]
failure, slightly modified

re comment 14: Interesting.  I get neither a hashchange nor a history entry for that testcase.  At least on my browser, it appears that setting location.hash='#' actually sets location.hash to '', as illustrated by the testcase I just uploaded.

I'm running the latest Shiretoko nightly on Linux. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1pre) Gecko/20090708 Shiretoko/3.5.1pre
Comment 16 Justin Lebar (not reading bugmail) 2009-07-08 20:49:46 PDT
Oops.  I meant to test this on the latest trunk nightly.  But I get the same results as above: Neither a hashchange nor a history entry for either of the testcases in comment 14 and comment 15.  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090708 Minefield/3.6a1pre
Comment 17 Dão Gottwald [:dao] 2009-07-10 02:33:59 PDT
This looks like a places bug, not a tabbrowser one. Why don't you fix it in places code?
Comment 18 John P Baker 2009-07-10 03:09:10 PDT
(In reply to comment #17)
> This looks like a places bug, not a tabbrowser one. Why don't you fix it in
> places code?

It seems to me to be a hybrid bug; Storing the favicons in the places database is currently done by tabbrowser so adding the fragment code here seems right; Storing the title is currently done in the docshell - as is firing the hashchange - but I am not sure that it should also store the title for fragments, so I put it here mostly for convenience.

I would be happy if we just do the icon here and punt the title somewhere else. But where?
Comment 19 Dão Gottwald [:dao] 2009-07-10 03:49:25 PDT
Rather than storing any extra data, why don't we use foo's title and favicon for foo#bar?
Comment 20 John P Baker 2009-07-10 04:01:48 PDT
(In reply to comment #18)
> Storing the title is currently done in the docshell - as is firing the
> hashchange - but I am not sure that it should also store the title for
> fragments, so I put it here mostly for convenience.
> 
> I would be happy if we just do the icon here and punt the title somewhere else.
> But where?

Having found the appropriate code for session history, the docShell probably _is_ the
correct place; Presumably something like:

7895     /* Set the title for the SH entry for this target url. so that
7896      * SH menus in go/back/forward buttons won't be empty for this.
7897      */
7898     if (mSessionHistory) {
7899         PRInt32 index = -1;
7900         mSessionHistory->GetIndex(&index);
7901         nsCOMPtr<nsIHistoryEntry> hEntry;
7902         mSessionHistory->GetEntryAtIndex(index, PR_FALSE,
7903                                          getter_AddRefs(hEntry));
7904         NS_ENSURE_TRUE(hEntry, NS_ERROR_FAILURE);
7905         nsCOMPtr<nsISHEntry> shEntry(do_QueryInterface(hEntry));
7906         if (shEntry)
7907             shEntry->SetTitle(mTitle);
7908     }
7909 
+        /* Set the title for the Global History entry for this target url. */
+        if (mGlobalHistory) {
+           mGlobalHistory->SetPageTitle(mCurrentURI, mTitle);
+        }
+
7910     if (doHashchange) {

But I would be out of my depth working here.
Comment 21 John P Baker 2009-07-10 05:36:11 PDT
Created attachment 387858 [details] [diff] [review]
patch v2a (favicon)

(In reply to comment #19)
> Rather than storing any extra data, why don't we use foo's title and favicon
> for foo#bar?

This was the approach in comment #4; The problem is you may never have visited foo (you may have got to foo#bar from foo#ey) which then mean that you have to create an entry for foo which (a) can disappear as there is no visit for it and
(b) creates a lot of database queries to track it.
Comment 22 Dão Gottwald [:dao] 2009-07-10 05:49:53 PDT
(In reply to comment #21)
> This was the approach in comment #4; The problem is you may never have visited
> foo (you may have got to foo#bar from foo#ey)

Well, that doesn't mean that you haven't visited foo before foo#ey. Also, if you visit foo#bar directly without a hash change, we store the favicon and the title. So the only remaining edge case is really visiting foo#ey directly (i.e. not foo) and then foo#bar. That's neither dramatic nor very common, and it doesn't justify the bloat your patch would add, imho.
Comment 23 John P Baker 2009-07-13 01:37:18 PDT
Created attachment 388194 [details] [diff] [review]
patch v3

Move handler to browser.js

The bad news is that is not so easy to find the relevant browser.
The good news is that the code is now a good match for the "icon" code in the DOMLinkAdded handler.
Comment 24 Dão Gottwald [:dao] 2009-07-13 01:53:18 PDT
Let my slightly rephrase an earlier question: Why does Places store random respectively no data for foo#bar, instead of using foo's title and favicon? Wouldn't it be nicer if that worked out of the box?
Comment 25 John P Baker 2009-07-17 05:37:50 PDT
(In reply to comment #20)

> Having found the appropriate code for session history, the docShell probably
> _is_ the correct place; Presumably something like:

...

> 7909 
> +        /* Set the title for the Global History entry for this target url. */
> +        if (mGlobalHistory) {
> +           mGlobalHistory->SetPageTitle(mCurrentURI, mTitle);
> +        }
> +
> 7910     if (doHashchange) {
> 
> But I would be out of my depth working here.

I am afraid that I have punted this to bug 503832 - so it will now need a c++ programmer.

(In reply to comment #24)
> Let my slightly rephrase an earlier question: Why does Places store random
> respectively no data for foo#bar, instead of using foo's title and favicon?
> Wouldn't it be nicer if that worked out of the box?

As used here places is just an implementation of nsIGlobalHistory2 so it is not reasonable for it to do anything but store and retrieve titles as requested by the browser; Note that Seamonkey 1.1.17 also exhibits the same behaviour as this bug - use STR from comment #0 and then Go/History - which convinces me it is a Core bug.

By analogy the icons are stored by the browser (on DOMLinkAdded in browser.js via setIcon in tabbrowser which calls the favicon service) and so the patch does the same on hashchange for the fragment URLs; Note that hashchange is fired just after the session history title is updated and hopefully where the global title will be.
Comment 26 John P Baker 2009-07-17 05:40:09 PDT
Created attachment 389111 [details] [diff] [review]
patch v4

Check aEvent.isTrusted

setAndLoadFaviconForPage checks isFailedIcon so don't do it here.
Comment 27 Dão Gottwald [:dao] 2009-07-17 06:10:56 PDT
(In reply to comment #25)
> As used here places is just an implementation of nsIGlobalHistory2 so it is not
> reasonable for it to do anything but store and retrieve titles as requested by
> the browser

I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the interface isn't sane, or maybe Places could interpret it more laxy.

> Note that Seamonkey 1.1.17 also exhibits the same behaviour as
> this bug - use STR from comment #0 and then Go/History - which convinces me it
> is a Core bug.

The fact that multiple consumers suffer from this actually seems to support that either the interface or the implementation should be fixed.

Btw, without being convinced that you're using the right approach, note that this structure is unnecessarily complicated:

const hashChangeHandler = {
  handleEvent: function (aEvent) {
    switch (aEvent.type) {
      case "hashchange":
        this.onHashChange(aEvent);
        break;
    }
  },
  onHashChange: function(aEvent) {
    FOO
  }
};

Use this instead:

function hashChangeHandler(aEvent) {
  FOO
}
Comment 28 Dão Gottwald [:dao] 2009-07-17 06:11:59 PDT
(In reply to comment #27)
> I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the
> interface isn't sane, or maybe Places could interpret it more laxy.

s/laxy/laxly/
Comment 29 Dão Gottwald [:dao] 2009-07-17 06:15:03 PDT
Comment on attachment 389111 [details] [diff] [review]
patch v4

>+    var browser = gBrowser.getBrowserForDocument(aEvent.target.document);
>+    if (!browser)
>+      return

How about:

if (aEvent.target != content)
  return;

var browser = gBrowser.selectedBrowser;
Comment 30 John P Baker 2009-07-17 07:25:55 PDT
Created attachment 389119 [details] [diff] [review]
patch v5

(In reply to comment #27)
> Btw, without being convinced that you're using the right approach, note that
> this structure is unnecessarily complicated:

Yes I think I must have been using 'this' at some stage which made it convenient.

(In reply to comment #29)

> How about:
> 
> if (aEvent.target != content)
>   return;
> 
> var browser = gBrowser.selectedBrowser;

I'm pretty sure that the event could fire on any browser so need to identify which one it was.
Comment 31 Dão Gottwald [:dao] 2009-07-17 08:04:38 PDT
It could, but even if you care about that case, you should still check the 'content' shortcut.
Comment 32 John P Baker 2009-07-23 05:38:11 PDT
Created attachment 390209 [details] [diff] [review]
patch v5.1
Comment 33 John P Baker 2009-07-29 01:08:48 PDT
Created attachment 391288 [details] [diff] [review]
patch v5.2
Comment 34 Gervase Markham [:gerv] 2009-11-26 06:17:09 PST
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
Comment 35 Philipp von Weitershausen [:philikon] 2010-03-11 11:04:09 PST
Created attachment 431922 [details] [diff] [review]
Test case and bugfix based on John's v5.2 patch

Apart from ruining the aesthetics of the affected history entries, this bug also causes problems for add-ons that query history and rely on description + favicon being correct (e.g. BarTab). So it'd be quite useful if we could decide whether John's patch was the correct way to proceed, especially with the docshell part (bug 503832) being fixed now (or soon to be fixed again ;)).

I have attached a patch against mozilla-central containing John's bugfix and a test case exercising the fix. Dietrich suggested Gavin as a reviewer in comment 13, so I'm submitting it to him. Cheers!
Comment 36 Philipp von Weitershausen [:philikon] 2010-03-18 07:57:15 PDT
Created attachment 433336 [details] [diff] [review]
Test case and bugfix, updated for bug 544097

Here's an updated version of the patch containing John's bugfix and my test case. The only difference between this and the previous version of the patch concerns bug 544097. Since that landed, tests are no longer supposed to use localhost:8888 but mochi.test:8888.

Would be cool if we can reach a verdict whether this bugfix can go in or not. I certainly wouldn't mind seeing this (and bug 503832) fixed for the Firefox 3.6.x line. ;)
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-03-25 09:13:16 PDT
It seems wasteful to have favicon entries for every fragment navigated to, and it kind of sucks to have to add this kind of code at this level (in the app vs. in the core).

Couldn't we instead change the favicon service to ignore the fragment when responding to queries, and just return the base page's favicon? That means we lose support for AJAXy pages that dynamically change their favicon based on the fragment, but I'm not sure that's really worth supporting, and doing the simpler thing now doesn't really preclude us from doing more later AFAICT.
Comment 38 Shawn Wilsher :sdwilsh 2010-03-25 09:27:01 PDT
(In reply to comment #37)
> Couldn't we instead change the favicon service to ignore the fragment when
> responding to queries, and just return the base page's favicon? That means we
> lose support for AJAXy pages that dynamically change their favicon based on the
> fragment, but I'm not sure that's really worth supporting, and doing the
> simpler thing now doesn't really preclude us from doing more later AFAICT.
This shouldn't be too hard to do, and I agree that it is the better approach.
Comment 39 Philipp von Weitershausen [:philikon] 2010-03-25 10:15:03 PDT
I'd be fine with that, and I certainly agree with the sentiment of rather not having this code in the app. Note that changing the favicon in JavaScript after having changed window.location.hash will actually store the right favicon right now (tested on FF 3.6) since that explicitly triggers a store in the favicon service. So as Gavin said, that "feature" would be lost. I personally don't feel strongly about that, so I'm ok with ditching it in the process.

Gavin's suggestion would basically boil down to the favicon service normalising URIs to the base URI before both storing and querying. The normalisation when storing is necessary because one might visit http://host/page#fragment and move on to other fragments before ever visiting http://host/page. I'll work on a patch for this.
Comment 40 John P Baker 2010-03-26 01:32:12 PDT
(In reply to comment #37)
> It seems wasteful to have favicon entries for every fragment navigated to, and
> it kind of sucks to have to add this kind of code at this level (in the app vs.
> in the core).

There is already a history entry and the favicon is probably already stored so there
isn't actually much extra stored - a favicon_id instead of a null.

> Couldn't we instead change the favicon service to ignore the fragment when
> responding to queries, and just return the base page's favicon? That means we
> lose support for AJAXy pages that dynamically change their favicon based on the
> fragment, but I'm not sure that's really worth supporting, and doing the
> simpler thing now doesn't really preclude us from doing more later AFAICT.

Some of the earlier experiments (see proof of concept attachments) on this bug tried that approach; The trouble is that there may not be an entry for the base page so that, on visit to http://host/page#fragment, they had to create an entry for http://host/page  which would be lost on browser shutdown as there was no visit. [This can also then delete the favicon data from the database as there may now be no reference to it]

I also suspect that the queries for history that include the favicon will become very hairy such that the right decision becomes to store the favicon relation when you know what it is - rather than trying to figure it out later.
Comment 41 dziastinux 2011-03-28 00:31:07 PDT
Here is another test case which fails in all Firefox versions (including 4.0):
http://jsbin.com/ulaxo4
Comment 42 Justin Lebar (not reading bugmail) 2011-05-20 14:19:09 PDT
Now that we've fixed bug 655270, we should be able to use the same approach to fixing this bug: On a hash load, call into the favicon service and tell it that the new favicon is the same as the old page's favicon.

John, do you mind if I pick up this bug?
Comment 43 Justin Lebar (not reading bugmail) 2011-05-20 14:22:57 PDT
(In reply to comment #37)
> It seems wasteful to have favicon entries for every fragment navigated to,

Maybe, but in bug 655270, we made it so each pushState causes a new favicon entry.  I don't think this is a lot different.

> and it kind of sucks to have to add this kind of code at this level (in the
> app vs. in the core).

I can move it into docshell if you'd prefer.
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-20 15:55:24 PDT
Yes, having this code in docshell would be better.
Comment 45 Justin Lebar (not reading bugmail) 2011-05-22 09:58:21 PDT
Created attachment 534310 [details] [diff] [review]
Patch v1 - Send notification from docshell
Comment 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-22 10:07:05 PDT
Comment on attachment 534310 [details] [diff] [review]
Patch v1 - Send notification from docshell

>diff --git a/docshell/test/browser/browser_bug420605.js b/docshell/test/browser/browser_bug420605.js

>+    gBrowser.selectedBrowser.addEventListener(
>+        "DOMContentLoaded", onPageLoad, true);

Probably a good idea to remove this listener at some point (even though you're removing the tab it's added on).

Looks great to me, but I'm not a docshell peer.
Comment 47 Justin Lebar (not reading bugmail) 2011-05-22 10:13:37 PDT
Comment on attachment 534310 [details] [diff] [review]
Patch v1 - Send notification from docshell

Jonas, do you want to review the docshell bits?
Comment 48 Justin Lebar (not reading bugmail) 2011-05-25 08:34:15 PDT
http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7
Comment 49 Philipp von Weitershausen [:philikon] 2011-05-25 09:16:32 PDT
(In reply to comment #48)
> http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7

Thanks, Justin, for finishing this up!

Note You need to log in before you can comment on or make changes to this bug.