Re: Move wallpaper progress bar on top of thumbnails (completely remove butter bar) (issue 12042095)

[fla...@chromium.org]

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/chromeos/wallpaper_manager/js/progress_manager.js
File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode32
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/chromeos/wallpaper_manager/js/progress_manager.js#newcode49
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/chromeos/wallpaper_manager/js/progress_manager.js
File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode21
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.)

https://codereview.chromium.org/12042095/

[bs...@chromium.org]

Done. PTAL. Thanks for review.

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

https://codereview.chromium.org/12042095/diff/13001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode49
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/chromeos/wallpaper_manager/js/progress_manager.js
File
chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js
(right):

https://codereview.chromium.org/12042095/diff/26001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode21
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.

[bs...@chromium.org]

In case this is buried. friendly ping? Thanks.

[fla...@chromium.org]

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

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode50
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.

https://codereview.chromium.org/12042095/

[bs...@chromium.org]

done. thanks!

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

https://codereview.chromium.org/12042095/diff/29001/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode50
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.

[fla...@chromium.org]

LGTM with nit.


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

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode36
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.

https://codereview.chromium.org/12042095/

[bs...@chromium.org]

Done. Thanks for review!

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

https://codereview.chromium.org/12042095/diff/19011/chrome/browser/resources/chromeos/wallpaper_manager/js/progress_manager.js#newcode36
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.

https://codereview.chromium.org/12042095/