Show a little circly/arrowey badge for files that have been copied locally.

Show a little circly/arrowey badge for files that have been copied locally.

Update import history code to support tracking locally copied files (and their destinations). Tracking destination is necessary as we'll eventually need to do lookups against destinations in order to mark items as imported when they are fully synced to the cloud.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/45e673e6ff837eeb500c2ad5bcbe5a799e8ba180
Cr-Commit-Position: refs/heads/master@{#310189}

[Steve McKay, 2015-01-06 01:09:07]

Add support for tracking local copy operations separately from cloud import.

[Steve McKay, 2015-01-06 01:11:02]

smckay@chromium.org changed reviewers:
+ kenobi@chromium.org, mtomasz@chromium.org

[mtomasz, 2015-01-06 03:51:01]

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/import_history.js (right):

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:29:
importer.ImportHistory.prototype.wasImported;
import.Destination is a destination for importing. Then, if we import with
DEVICE argument, then I ex

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:128:
importer.CopyData_;
nit: I'm not sure here, but does it make sense to make a type private?

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:129: 
nit: Redundant \n.

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:296: * @param
{importer.CopyData_?} opt_copyData
nit: ? -> =

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:302: 
nit: \n redundant.

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:310: // copy all
entries
nit: copy -> Copy
nit: entries -> entries.

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:311: for (var dest
in opt_copyData) {
nit: Abbreviations are discouraged by our style guide. Especially here
difference between dest and destination is confusing. Can we choose a better
name?

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:365:
function(entry, destination, url) {
What happens if we call markCopied with DEVICE destination? Does it make sense?
I'm wondering if we should have DEVICE in importer.Destination, as the logic for
it is different than for real import destinations. WDYT?

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:377: return
this.addDestination_(
I find this a little bit complex. We get a call markCopied with GOOGLE_DRIVE
destination, but then we call addDestination with DEVICE destination.

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:437: * @param
{importer.CopyData_?} opt_copyData
nit: ditto

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:453: * @return
{!Array.<string>} The list of previously noted
nit: string -> importer.Destination

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:32: // locally copied,
but not imported to cloud as of yet.
nit: locally -> Locally

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/837563002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:303: // "box" is
currently the sibling of the elemement
typo: element

[mtomasz, 2015-01-06 04:18:12]

Per offline discussion. After adding record type instead of overriding
import.Destination lgtm with nits as long as lgtm by @kenobi after the change.

[Steve McKay, 2015-01-06 20:15:28]

Update record storage to use an explicit RecordType.

[Steve McKay, 2015-01-06 20:28:49]

Update FileGrid to use new copy logic.

[Ben Kwa, 2015-01-06 21:57:50]

lgtm

Discussed offline, remaining issues are noted, otherwise, LG.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/import_history.js (right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:212: for (var path
in entries[key][destination]) {
There should be no third nested loop, only 1 path per destination.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:310:
this.copiedEntries_[key][destination] = opt_payload[destination];
This makes opt_payload not really an optional parameter.  It also treats
opt_payload as an Object type, which contradicts the type annotation above.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:337: * @return
{!Promise.<boolean>}
Should be {boolean}, not {Promise<boolean>}.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/import_history_unittest.js
(right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history_unittest.js:67:
.catch(handleError.bind(null, callback));
Might be more clearly written as

reportPromise(
  historyProvider.then(
      function(history) {
        history.wasCopied(testFileEntry, SPACE_CAMP).then(assertFalse);
      }));

Similarly for the ones below.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:307:
box.parentElement.classList.add('copied');
'copied', or 'imported'?

[Steve McKay, 2015-01-06 22:27:27]

Varous fixups. Respond to review comments. Update tests to use reportPromise.

[Steve McKay, 2015-01-06 22:27:59]

Dones. Thanks!

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/import_history.js (right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:212: for (var path
in entries[key][destination]) {
On 2015/01/06 21:57:50, Ben Kwa wrote:
> There should be no third nested loop, only 1 path per destination.

Done.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:310:
this.copiedEntries_[key][destination] = opt_payload[destination];
On 2015/01/06 21:57:50, Ben Kwa wrote:
> This makes opt_payload not really an optional parameter.  It also treats
> opt_payload as an Object type, which contradicts the type annotation above.

Fixed. Just a simple value.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history.js:337: * @return
{!Promise.<boolean>}
On 2015/01/06 21:57:50, Ben Kwa wrote:
> Should be {boolean}, not {Promise<boolean>}.

Done.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/import_history_unittest.js
(right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/import_history_unittest.js:67:
.catch(handleError.bind(null, callback));
On 2015/01/06 21:57:50, Ben Kwa wrote:
> Might be more clearly written as
> 
> reportPromise(
>   historyProvider.then(
>       function(history) {
>         history.wasCopied(testFileEntry, SPACE_CAMP).then(assertFalse);
>       }));
> 
> Similarly for the ones below.

Done.

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/837563002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:307:
box.parentElement.classList.add('copied');
On 2015/01/06 21:57:50, Ben Kwa wrote:
> 'copied', or 'imported'?

Updated, but yes, copied. If imported, we don't show copied...so this comes only
if not imported.

[Steve McKay, 2015-01-06 22:28:05]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-01-06 22:29:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837563002/80001

[Steve McKay, 2015-01-06 23:26:16]

Add copy support to test import_history and update media import handler test to
check for copy results instead of import.

[Steve McKay, 2015-01-06 23:27:37]

Add copy support to test import_history and update media import handler test to
check for copy results instead of import.

[Steve McKay, 2015-01-07 00:15:32]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-01-07 00:16:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837563002/120001

[commit-bot: I haz the power, 2015-01-07 00:30:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-07 00:32:04]

Patchset 7 (id:??) landed as
https://crrev.com/45e673e6ff837eeb500c2ad5bcbe5a799e8ba180
Cr-Commit-Position: refs/heads/master@{#310189}