(NOT FOR FINAL REVIEW)Refactor butter_bar.js for wallpaper manager and file manager to use.

Refactor butter_bar.js for wallpaper manager and file manager to use.


Wallpaper manager also wants to use the same butter bar as file manager. Factor
out butter_bar.js from file manager to shared/js/chromeos/butter_bar.js


BUG=None

[bshe, 2012-09-06 18:00:07]

Hi Oleg.

From git log, it seems you factored butter_bar.js out from file_manager.js. Now
it seems we want to go one step further. Wallpaper manager also want to use
butter bar in the UI. So this Cl basically want to factor out some non file
manage specific butter bar method to shared/js/chromeos/butter_bar.js. Wallpaper
manager can then reuse them.

This CL is not for final review yet. Just want to get some early feedback (is it
the correct way to refactor) from you before I do more work.

Thanks!

[Oleg Eterevsky, 2012-09-07 08:43:50]

I like you CL. It looks like a clear separation into UI and copy progress logic.
Will you try to move CSS to shared resources too? It's in
file_manager/css/file_manager.css.

When you do you plan to land these changes? Do you want to make them in M23?

http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager_butter_bar.js
(right):

http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager_butter_bar.js:20:
FileManagerButterBar.prototype = {
In FileManager we use a different style for class declarations, with each method
in a separate assignment. This way we get warnings about errors in jsdoc.

[Vladislav Kaznacheev, 2012-09-07 12:41:54]

I do not feel that ButterBar class qualifies for the shared status. My reasons:

1. It is small (178 lines)
2. It is not "magical" enough to convince me to use it instead of rolling my own
(unlike cr.ui.List/DataModel/SelectionModel combo which solves a complex problem
so well that I am willing to live with its quirks)
3. It does what it does in a fairly peculiar way. What are the chances that
there will be another client who wants exactly this feature set?
4. It is not stable enough. The FileManager team has a short-term plan to
rewrite it to make it reusable in the Photo Editor. I would not like us to be
constrained with (artifically) fixed API that Wallpaper manager depends on.

I suggest that you copy these 178 lines to your codebase for now. Lets get back
to this in a month or so when we know what we want from our butter bar and see
if there is enough grounds for sharing the code.





On 2012/09/07 08:43:50, Oleg wrote:
> I like you CL. It looks like a clear separation into UI and copy progress
logic.
> Will you try to move CSS to shared resources too? It's in
> file_manager/css/file_manager.css.
> 
> When you do you plan to land these changes? Do you want to make them in M23?
> 
>
http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
> File chrome/browser/resources/file_manager/js/file_manager_butter_bar.js
> (right):
> 
>
http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
> chrome/browser/resources/file_manager/js/file_manager_butter_bar.js:20:
> FileManagerButterBar.prototype = {
> In FileManager we use a different style for class declarations, with each
method
> in a separate assignment. This way we get warnings about errors in jsdoc.

[bshe, 2012-09-07 14:03:33]

Thanks for comments and suggestions.

I am hoping to land butter bar for wallpaper manager in M23. But it looks
controversial to abstract that class now as Vladislav argued.

So for M23, I would go whatever easiest(copy these lines to wallpaper manager)
considering the time remaining. And leave a TODO and get back to it once butter
bar is finalized. 

On 2012/09/07 12:41:54, Vladislav Kaznacheev wrote:
> I do not feel that ButterBar class qualifies for the shared status. My
reasons:
> 
> 1. It is small (178 lines)
> 2. It is not "magical" enough to convince me to use it instead of rolling my
own
> (unlike cr.ui.List/DataModel/SelectionModel combo which solves a complex
problem
> so well that I am willing to live with its quirks)
> 3. It does what it does in a fairly peculiar way. What are the chances that
> there will be another client who wants exactly this feature set?
> 4. It is not stable enough. The FileManager team has a short-term plan to
> rewrite it to make it reusable in the Photo Editor. I would not like us to be
> constrained with (artifically) fixed API that Wallpaper manager depends on.
> 
> I suggest that you copy these 178 lines to your codebase for now. Lets get
back
> to this in a month or so when we know what we want from our butter bar and see
> if there is enough grounds for sharing the code.
> 
> 
> 
> 
> 
> On 2012/09/07 08:43:50, Oleg wrote:
> > I like you CL. It looks like a clear separation into UI and copy progress
> logic.
> > Will you try to move CSS to shared resources too? It's in
> > file_manager/css/file_manager.css.
> > 
> > When you do you plan to land these changes? Do you want to make them in M23?
> > 
> >
>
http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
> > File chrome/browser/resources/file_manager/js/file_manager_butter_bar.js
> > (right):
> > 
> >
>
http://codereview.chromium.org/10916149/diff/2001/chrome/browser/resources/fi...
> > chrome/browser/resources/file_manager/js/file_manager_butter_bar.js:20:
> > FileManagerButterBar.prototype = {
> > In FileManager we use a different style for class declarations, with each
> method
> > in a separate assignment. This way we get warnings about errors in jsdoc.