Fix CSS and timeout handling for the Files app butter bar

Fix CSS and timeout handling for the Files app butter bar


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

[Oleg Eterevsky, 2012-08-03 07:36:13]

LGTM with minor comments.

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/butter_bar.js (right):

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:54: if
(this.hideTimeout_) {
These 4 lines appear at least 3 times. Maybe extract them into small method
clearHideTimeout()?

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:68:
link.addEventListener('click', function(callback) {
Why did you change the way this function is written? If you are worried that
opt_options.actions[label] may be overwritten before the event listener is
called, I suggest you write:

var callback = opt_options.actions[label];
link.addEventListener('click', function() { callback(); return false; });

It's much easier to understand.

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:76: actions.hidden =
true;
Could you please add braces here? Otherwise it look a bit weird. I believe it's
ok to skip braces only for one-liners like

if (something)
  do_something();

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:107: var timeout =
('timeout' in opt_options) ? opt_options.timeout : 10 * 1000;
Could you please swap this line and the next if, so that timeout declaration
would be near its use?

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:117: this.hide_();
I missed this while refactoring. Thanks!

[Vladislav Kaznacheev, 2012-08-03 08:20:04]

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/butter_bar.js (right):

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:54: if
(this.hideTimeout_) {
On 2012/08/03 07:36:13, Oleg wrote:
> These 4 lines appear at least 3 times. Maybe extract them into small method
> clearHideTimeout()?

Done.

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:68:
link.addEventListener('click', function(callback) {
A closure inside a loop is bound to the last valuer of the loop variable.
On 2012/08/03 07:36:13, Oleg wrote:
> Why did you change the way this function is written? If you are worried that
> opt_options.actions[label] may be overwritten before the event listener is
> called, I suggest you write:
> 
> var callback = opt_options.actions[label];
> link.addEventListener('click', function() { callback(); return false; });
> 
> It's much easier to understand.

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:76: actions.hidden =
true;
On 2012/08/03 07:36:13, Oleg wrote:
> Could you please add braces here? Otherwise it look a bit weird. I believe
it's
> ok to skip braces only for one-liners like
> 
> if (something)
>   do_something();

Done.

https://chromiumcodereview.appspot.com/10826137/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/butter_bar.js:107: var timeout =
('timeout' in opt_options) ? opt_options.timeout : 10 * 1000;
On 2012/08/03 07:36:13, Oleg wrote:
> Could you please swap this line and the next if, so that timeout declaration
> would be near its use?

Done.