Move FileCopyManager to background page.

Move FileCopyManager to background page.

BUG=126810
TEST=Copy/paste and drug&drop operations.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148535

[dgozman, 2012-07-12 13:17:36]

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/background_main.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/background_main.js:5:
metrics.recordInterval('Load.Script');  // Must be the last line.
Do we need this code along with file?

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_copy_manager.js:261:
w.fileCopyManagerWrapper.onEvent(eventName, eventArgs);
Why can't you send the event directly to window.fileManager instance?

Also, I don't quite like one more level of indirection. CopyManager should have
2 or 3 public methods, and creating a whole wrapper is an overkill.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_copy_manager.js:666:
this.log_('serviceNextTaskEntry error: ' + reason + ':', data);
Are you sure that console.log does not work for background page?

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:17: this.console_ =
console;
Not used.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4669:
chrome.extension.getBackgroundPage().FileCopyManager.prototype;
Does this work with unloaded background page?

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4710: for (var arg in
eventArgs)
if (eventArgs.hasOwnProperty(arg))

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/manifest.json (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/manifest.json:154: "scripts":
["js/background_scripts.js"]
Including this file clearly does not work with load-component-extension.
Instead, you may list all needed files here in scripts list and add them to
component_resources.grd.

[Oleg Eterevsky, 2012-07-12 14:49:22]

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/background_main.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/background_main.js:5:
metrics.recordInterval('Load.Script');  // Must be the last line.
On 2012/07/12 13:17:36, dgozman wrote:
> Do we need this code along with file?

Removed it.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_copy_manager.js:261:
w.fileCopyManagerWrapper.onEvent(eventName, eventArgs);
On 2012/07/12 13:17:36, dgozman wrote:
> Why can't you send the event directly to window.fileManager instance?
>
> Also, I don't quite like one more level of indirection. CopyManager should
have
> 2 or 3 public methods, and creating a whole wrapper is an overkill.

From my point of view, it's fine to create a small class only to handle
interaction with the background page. FileManager class is already bloated.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_copy_manager.js:666:
this.log_('serviceNextTaskEntry error: ' + reason + ':', data);
On 2012/07/12 13:17:36, dgozman wrote:
> Are you sure that console.log does not work for background page?

At least it didn't work for me. The only messages I saw on the console were
error messages from the vm.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:17: this.console_ =
console;
On 2012/07/12 13:17:36, dgozman wrote:
> Not used.

Done.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4669:
chrome.extension.getBackgroundPage().FileCopyManager.prototype;
On 2012/07/12 13:17:36, dgozman wrote:
> Does this work with unloaded background page?

I wasn't able to test it.

https://chromiumcodereview.appspot.com/10692090/diff/2001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4710: for (var arg in
eventArgs)
On 2012/07/12 13:17:36, dgozman wrote:
> if (eventArgs.hasOwnProperty(arg))

Done.

[Oleg Eterevsky, 2012-07-17 15:19:06]

Could you give another look? I'm under impression that barring inability to
unload, the CL works fine.

[dgozman, 2012-07-17 16:10:53]

Well, this looks good. But I don't think you want to commit until transient
background page works.

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
chrome/browser/resources/file_manager/js/file_manager.js:4674: * Extending
cr.EventTarget
full stop

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
chrome/browser/resources/file_manager/js/main_scripts.js:56: //<include
src="file_copy_manager.js"/>
No need in this?

[Oleg Eterevsky, 2012-07-17 16:44:13]

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
chrome/browser/resources/file_manager/js/file_manager.js:4674: * Extending
cr.EventTarget
On 2012/07/17 16:10:53, dgozman wrote:
> full stop

Done.

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10692090/diff/12002/chrome/browser/res...
chrome/browser/resources/file_manager/js/main_scripts.js:56: //<include
src="file_copy_manager.js"/>
On 2012/07/17 16:10:53, dgozman wrote:
> No need in this?

Done.

[Oleg Eterevsky, 2012-07-24 15:29:53]

The background page now works at least partly correct: it does not load until
the first copy operation. Sadly, it doesn't unload afterwords.

[Oleg Eterevsky, 2012-07-24 16:03:34]

Ok, now the background page loads and unloads just fine.

[Oleg Eterevsky, 2012-07-24 16:04:23]

Vlad, Dmitry, now that Dmitry Gozman is on vacations, could you please review
this CL?

[Vladislav Kaznacheev, 2012-07-25 16:44:14]

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
File chrome/browser/resources/component_extension_resources.grd (right):

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/component_extension_resources.grd:42: <include
name="IDR_FILE_MANAGER_FILE_COPY_MANAGER_JS"
file="file_manager/js/file_copy_manager.js" flattenhtml="false" type="BINDATA"
/>
Lets move group of file below the flattened pages/scripts and add a comment such
as "Background page scripts"

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
File chrome/browser/resources/file_manager/photo_import.html (right):

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/file_manager/photo_import.html:22: Keep the list in
sync with photoapp_scripts.js. -->
Wrong file name, please fix.

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/file_manager/photo_import.html:45: <script
src="js/file_copy_manager_wrapper.js"></script>
You forgot to update photo_import_scripts.js.

[Oleg Eterevsky, 2012-07-25 17:26:40]

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
File chrome/browser/resources/component_extension_resources.grd (right):

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/component_extension_resources.grd:42: <include
name="IDR_FILE_MANAGER_FILE_COPY_MANAGER_JS"
file="file_manager/js/file_copy_manager.js" flattenhtml="false" type="BINDATA"
/>
On 2012/07/25 16:44:14, Vladislav Kaznacheev wrote:
> Lets move group of file below the flattened pages/scripts and add a comment
such
> as "Background page scripts"

Done.

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
File chrome/browser/resources/file_manager/photo_import.html (right):

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/file_manager/photo_import.html:22: Keep the list in
sync with photoapp_scripts.js. -->
On 2012/07/25 16:44:14, Vladislav Kaznacheev wrote:
> Wrong file name, please fix.

Done.

https://chromiumcodereview.appspot.com/10692090/diff/31001/chrome/browser/res...
chrome/browser/resources/file_manager/photo_import.html:45: <script
src="js/file_copy_manager_wrapper.js"></script>
On 2012/07/25 16:44:14, Vladislav Kaznacheev wrote:
> You forgot to update photo_import_scripts.js. 

Done.

[Vladislav Kaznacheev, 2012-07-25 17:27:46]

lgtm

[Oleg Eterevsky, 2012-07-25 17:30:27]

Please review component_extension_resources.grd. I've already got LGTM for all
the other files.

[James Hawkins, 2012-07-25 17:31:22]

LGTM

[Oleg Eterevsky, 2012-07-25 17:32:32]

Thanks for the review!

[commit-bot: I haz the power, 2012-07-25 17:32:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/10692090/28003

[commit-bot: I haz the power, 2012-07-25 18:04:13]

Try job failure for 10692090-28003 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...