Move wallpaper progress bar on top of thumbnails (completely remove butter bar)

Move wallpaper progress bar on top of thumbnails (completely remove butter bar)


This CL removes butter bar completely. We now display progress within each
thumbnail. And the UI for error message are removed compeltely for now.
After we figure out how to display these error messages. I will add them back
to UI.


BUG=162563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181544

[bshe, 2013-01-25 04:35:05]

Hi Rob.

Could you please take a look at this CL？
In this CL, I did the following:
1. Removed butter_bar completely. (note the error message are all disabled.
After we have a clear UI design of how to show these messages in the new UI, I
will then add these messages back)
2. Added a progress_manager.js to display downloading progress in the click
thumbnail.
3. Implemented the logic to add/remove progress bar and checkmark. (At any time
in a tab, there should be at most one instance of them). And checkmark is always
on the thumbnail of currently using wallpaper. 

Thanks!

Biao

https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:11:
this.xhr_ = null;
Instead of creating a progressBar for each thumbnail grid and toggle their
visiblity. We can also cache a progressBar in javascript and dynamically
add/remove it from thumbnail grid item. This approach maybe more efficient when
we have hundreds of thumbnails?

https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(left):

https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:36:
cr.defineProperty(this, 'active', cr.PropertyKind.BOOL_ATTR);
Also removed checkMark here for every item. And dynamically add/remove a cached
checkmark to thumbnail grid item

[bshe, 2013-01-31 00:48:51]

I have made some tweaks proposed by UX. Rob, could you take a look?
Thanks!

On 2013/01/25 04:35:05, bshe wrote:
> Hi Rob.
> 
> Could you please take a look at this CL？
> In this CL, I did the following:
> 1. Removed butter_bar completely. (note the error message are all disabled.
> After we have a clear UI design of how to show these messages in the new UI, I
> will then add these messages back)
> 2. Added a progress_manager.js to display downloading progress in the click
> thumbnail.
> 3. Implemented the logic to add/remove progress bar and checkmark. (At any
time
> in a tab, there should be at most one instance of them). And checkmark is
always
> on the thumbnail of currently using wallpaper. 
> 
> Thanks!
> 
> Biao
> 
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
> (right):
> 
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:11:
> this.xhr_ = null;
> Instead of creating a progressBar for each thumbnail grid and toggle their
> visiblity. We can also cache a progressBar in javascript and dynamically
> add/remove it from thumbnail grid item. This approach maybe more efficient
when
> we have hundreds of thumbnails?
> 
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> File
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
> (left):
> 
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:36:
> cr.defineProperty(this, 'active', cr.PropertyKind.BOOL_ATTR);
> Also removed checkMark here for every item. And dynamically add/remove a
cached
> checkmark to thumbnail grid item

[bshe, 2013-02-01 19:45:38]

Hi Rob. Could you please take a look?

On 2013/01/31 00:48:51, bshe wrote:
> I have made some tweaks proposed by UX. Rob, could you take a look?
> Thanks!
> 
> On 2013/01/25 04:35:05, bshe wrote:
> > Hi Rob.
> > 
> > Could you please take a look at this CL？
> > In this CL, I did the following:
> > 1. Removed butter_bar completely. (note the error message are all disabled.
> > After we have a clear UI design of how to show these messages in the new UI,
I
> > will then add these messages back)
> > 2. Added a progress_manager.js to display downloading progress in the click
> > thumbnail.
> > 3. Implemented the logic to add/remove progress bar and checkmark. (At any
> time
> > in a tab, there should be at most one instance of them). And checkmark is
> always
> > on the thumbnail of currently using wallpaper. 
> > 
> > Thanks!
> > 
> > Biao
> > 
> >
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> > File
> chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> >
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:11:
> > this.xhr_ = null;
> > Instead of creating a progressBar for each thumbnail grid and toggle their
> > visiblity. We can also cache a progressBar in javascript and dynamically
> > add/remove it from thumbnail grid item. This approach maybe more efficient
> when
> > we have hundreds of thumbnails?
> > 
> >
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> > File
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
> > (left):
> > 
> >
>
https://codereview.chromium.org/12042095/diff/11001/chrome/browser/resources/...
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:36:
> > cr.defineProperty(this, 'active', cr.PropertyKind.BOOL_ATTR);
> > Also removed checkMark here for every item. And dynamically add/remove a
> cached
> > checkmark to thumbnail grid item

[flackr, 2013-02-05 01:24:19]

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:12:
this.progressBar_ = cr.doc.createElement('div');
Could we avoid constructing the progress bar, perhaps template the construction?

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:21: *
Sets the XMLHttpRequest |xhr| to monitor and the wallpaper thumbnail grid
s/monitor/monitor,

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:22: *
item |selectedGridItem| to show progress bar. Cancels previous xhr and
s/progress bar/a progress bar for

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:32:
this.xhr_.abort();
this seems potentially dangerous if it runs. Will calling this.xhr_.abort issue
an 'abort' event? If the event callback is asynchronous it could unset this.xhr_
after the new xhr has been registered.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:49: *
Removes the progress bar in |selectedGridItem| if any. May called
s/May/May be

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:54:
ProgressManager.prototype.hide = function(selectedGridItem) {
You're always hiding the selected grid item's progress bar. Remove the argument
and just hide this.selectedGridItem_. nit: call this hideProgressBar to be
explicit.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:63:
ProgressManager.prototype.showProgress_ = function() {
How about setProgress_ = function(percentComplete) { since every time you call
showProgress you set this.percentComplete_ just before calling it? You don't
even need to store the progress just update the progress-track to this length.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:95:
this.progressBar_.querySelector('.progress-track').style.width = '100%';
Call this.setProgress_(1);

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:460:
null));
Remove the null, this will pass undefined instead.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:466:
* @param {WallpaperThumbnailsGridItem} selectedGridItem The wallpaper
Optional parameter: @param {WallpaperThumbnailsGridItem=} opt_selectedGridItem

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:575:
var previousCheckmark = $('wallpaper-grid').querySelector('.check');
Seems like previousCheckmark == this.checkmark_, in which case you don't have to
use a querySelector to find it, you can just detach it from its parent if it's
attached.

[bshe, 2013-02-05 17:33:10]

Thanks for review. Please see inlined comment.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:12:
this.progressBar_ = cr.doc.createElement('div');
Created a template in html.

I was trying to make ProgressManager self contained. So I constructed the
progressBar object in this constructor.

On 2013/02/05 01:24:20, flackr wrote:
> Could we avoid constructing the progress bar, perhaps template the
construction?

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:21: *
Sets the XMLHttpRequest |xhr| to monitor and the wallpaper thumbnail grid
On 2013/02/05 01:24:20, flackr wrote:
> s/monitor/monitor,

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:22: *
item |selectedGridItem| to show progress bar. Cancels previous xhr and
On 2013/02/05 01:24:20, flackr wrote:
> s/progress bar/a progress bar for

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:32:
this.xhr_.abort();
the abort will issue an 'abort' event. And the event handler seems synchronous.
I also stepped into the abort function using dev tool. And it seems it calls
onDownloadAbort_ before return. So it is probably OK here?

On 2013/02/05 01:24:20, flackr wrote:
> this seems potentially dangerous if it runs. Will calling this.xhr_.abort
issue
> an 'abort' event? If the event callback is asynchronous it could unset
this.xhr_
> after the new xhr has been registered.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:49: *
Removes the progress bar in |selectedGridItem| if any. May called
On 2013/02/05 01:24:20, flackr wrote:
> s/May/May be

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:54:
ProgressManager.prototype.hide = function(selectedGridItem) {
I have changed the function name. But this function is also called by
onFinished_ function in wallpaper_manager.js. And the onFinished_ is an
asynchronous callback function of setWallpaper. If we always hide
selectedGridItem_, we may hide the new one accidentally.

On 2013/02/05 01:24:20, flackr wrote:
> You're always hiding the selected grid item's progress bar. Remove the
argument
> and just hide this.selectedGridItem_. nit: call this hideProgressBar to be
> explicit.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:63:
ProgressManager.prototype.showProgress_ = function() {
On 2013/02/05 01:24:20, flackr wrote:
> How about setProgress_ = function(percentComplete) { since every time you call
> showProgress you set this.percentComplete_ just before calling it? You don't
> even need to store the progress just update the progress-track to this length.

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:95:
this.progressBar_.querySelector('.progress-track').style.width = '100%';
On 2013/02/05 01:24:20, flackr wrote:
> Call this.setProgress_(1);

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:460:
null));
On 2013/02/05 01:24:20, flackr wrote:
> Remove the null, this will pass undefined instead.

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:466:
* @param {WallpaperThumbnailsGridItem} selectedGridItem The wallpaper
On 2013/02/05 01:24:20, flackr wrote:
> Optional parameter: @param {WallpaperThumbnailsGridItem=} opt_selectedGridItem

Done.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:575:
var previousCheckmark = $('wallpaper-grid').querySelector('.check');
On 2013/02/05 01:24:20, flackr wrote:
> Seems like previousCheckmark == this.checkmark_, in which case you don't have
to
> use a querySelector to find it, you can just detach it from its parent if it's
> attached.

Done.

[flackr, 2013-02-06 15:30:46]

This looks good, I just have a reservation about two classes controlling the
same xhr. Either progress manager should create the xhr, or take ownership of
the xhr, or just observe the xhr (i.e. not abort it, just stop listening).

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:32:
this.xhr_.abort();
Sounds good, just wanted to be sure.

On 2013/02/05 17:33:10, bshe wrote:
> the abort will issue an 'abort' event. And the event handler seems
synchronous.
> I also stepped into the abort function using dev tool. And it seems it calls
> onDownloadAbort_ before return. So it is probably OK here?
> 
> On 2013/02/05 01:24:20, flackr wrote:
> > this seems potentially dangerous if it runs. Will calling this.xhr_.abort
> issue
> > an 'abort' event? If the event callback is asynchronous it could unset
> this.xhr_
> > after the new xhr has been registered.
>

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:49: *
Removes the progress bar in |selectedGridItem| if any. May called
Sorry, the space was intentional, s/Maybe/May be

On 2013/02/05 17:33:10, bshe wrote:
> On 2013/02/05 01:24:20, flackr wrote:
> > s/May/May be
> 
> Done.

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:21: *
loadstart event.
The semantics of passing the XHR in and controlling some aspects of it here but
others in wallpaper_manager seems a little tricky. This should either strictly
observe the XHR (i.e. not cancel them, and unregister on XHR's it's no longer
interested in) or take control of the XHR (i.e. this method calls send, perhaps
even construct the XHR here and you just pass in the request URL and add a
listener on progress manager for completion.)

[bshe, 2013-02-06 17:51:31]

Done. PTAL. Thanks for review.

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:49: *
Removes the progress bar in |selectedGridItem| if any. May called
oops. Sorry. Done.

On 2013/02/06 15:30:46, flackr wrote:
> Sorry, the space was intentional, s/Maybe/May be
> 
> On 2013/02/05 17:33:10, bshe wrote:
> > On 2013/02/05 01:24:20, flackr wrote:
> > > s/May/May be
> > 
> > Done.
>

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:21: *
loadstart event.
You are right. And it seems we already did the abort call in wallpaper manager.
So no need to call it again here.

I removed the abort function and add function to unregister all events for
previous xhr if any. Now progress manager should only strictly observer XHR. 

On 2013/02/06 15:30:46, flackr wrote:
> The semantics of passing the XHR in and controlling some aspects of it here
but
> others in wallpaper_manager seems a little tricky. This should either strictly
> observe the XHR (i.e. not cancel them, and unregister on XHR's it's no longer
> interested in) or take control of the XHR (i.e. this method calls send,
perhaps
> even construct the XHR here and you just pass in the request URL and add a
> listener on progress manager for completion.)

[bshe, 2013-02-07 20:54:59]

In case this is buried. friendly ping? Thanks.

On 2013/02/06 17:51:31, bshe wrote:
> Done. PTAL. Thanks for review.
> 
>
https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
> (right):
> 
>
https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/...
> chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:49:
*
> Removes the progress bar in |selectedGridItem| if any. May called
> oops. Sorry. Done.
> 
> On 2013/02/06 15:30:46, flackr wrote:
> > Sorry, the space was intentional, s/Maybe/May be
> > 
> > On 2013/02/05 17:33:10, bshe wrote:
> > > On 2013/02/05 01:24:20, flackr wrote:
> > > > s/May/May be
> > > 
> > > Done.
> >
> 
>
https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
> (right):
> 
>
https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/...
> chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:21:
*
> loadstart event.
> You are right. And it seems we already did the abort call in wallpaper
manager.
> So no need to call it again here.
> 
> I removed the abort function and add function to unregister all events for
> previous xhr if any. Now progress manager should only strictly observer XHR. 
> 
> On 2013/02/06 15:30:46, flackr wrote:
> > The semantics of passing the XHR in and controlling some aspects of it here
> but
> > others in wallpaper_manager seems a little tricky. This should either
strictly
> > observe the XHR (i.e. not cancel them, and unregister on XHR's it's no
longer
> > interested in) or take control of the XHR (i.e. this method calls send,
> perhaps
> > even construct the XHR here and you just pass in the request URL and add a
> > listener on progress manager for completion.)

[flackr, 2013-02-07 21:00:16]

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:50:
this.onDownloadStart_.bind(this));
Sorry, I'm pretty sure this won't work. When you call .bind it creates a
function in the current closure, whose address won't match the other function
you created earlier. I believe you have to save the bound functions and cancel
those, but correct me if I'm mistaken.

[bshe, 2013-02-07 21:45:58]

done. thanks!

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:50:
this.onDownloadStart_.bind(this));
dooh. You are absolutely right, bind creates new functions. This is a silly
mistake. I tested my patch and everything looks fine so I assumed it works. I
guess the mistake is actually covered by the fact that we call abort in
wallpaper manager and then discard the previous xhr object so even if these
event listeners still exist, they wont do anything.

Anyway, I have made the change.

On 2013/02/07 21:00:16, flackr wrote:
> Sorry, I'm pretty sure this won't work. When you call .bind it creates a
> function in the current closure, whose address won't match the other function
> you created earlier. I believe you have to save the bound functions and cancel
> those, but correct me if I'm mistaken.

[flackr, 2013-02-07 22:01:53]

LGTM with nit.

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:36:
this.loadHandler_ = this.onDownloadComplete_.bind(this);
Nit: Can we clean this up a little? i.e.
this.xhrListeners_ = {
  'loadstart': this.onDownloadStart_.bind(this),
  ...
};

for (var eventType in this.xhrListeners)
  this.xhr_.addEventListener(eventType, this.xhrListeners[type]);

and similar for remove? Then we don't have to worry about keeping the remove
list in sync.

[bshe, 2013-02-07 23:07:47]

Done. Thanks for review!

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js:36:
this.loadHandler_ = this.onDownloadComplete_.bind(this);
On 2013/02/07 22:01:53, flackr wrote:
> Nit: Can we clean this up a little? i.e.
> this.xhrListeners_ = {
>   'loadstart': this.onDownloadStart_.bind(this),
>   ...
> };
> 
> for (var eventType in this.xhrListeners)
>   this.xhr_.addEventListener(eventType, this.xhrListeners[type]);
> 
> and similar for remove? Then we don't have to worry about keeping the remove
> list in sync.

Done.