Files.app: Add deduplication code for imports to Google Drive.

Files.app: Add deduplication code for imports to Google Drive.

Add calls to hash generation and comparison code so that files being imported
into Google Drive are actually deduplicated.

BUG=420680
TEST=FileManagerJsTest.DuplicateFinderTest FileManagerJsTest.MediaImportHandlerTest

Committed: https://crrev.com/3244b994fd8c52b595b3bc101a7e1ed77b04c0f0
Cr-Commit-Position: refs/heads/master@{#313621}

[Ben Kwa, 2015-01-28 16:57:57]

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

[Steve McKay, 2015-01-28 18:17:40]

LGTM w/ nits.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:51: return new
Promise(function(resolve, reject) {
Please wrap before anon functions...it is *very* hard to read when inlined.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:54:
function(result) {
@param

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:55: if
(!chrome.runtime.lastError) {
The logic is unnecessarily twisted here.

if (chrome.runtime.lastError) {
  reject(chrome.runtime.lastError);
} else {
  resolve(result);
}

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:80:
importer.DriveDuplicateFinder.prototype.getDriveId_ = function() {
A comment intended to influence your future design decisions:

Drive ID is something that should be injected. That is obviously a pain given
the arrangement with how this class is constructed. I think the second best
thing would have been to inject a Promise.<string>.

Doing so would have kept the drive ID lookup logic separate from the core dedupe
logic and in doing so reduced the number of false failures that can happen on
this test. It would also have made this class MUCH easier to test given there'd
be no need for a MockFileSystem at all.

Lastly it would eliminate that funky null promise that is lazily initialized.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:102:
importer.DriveDuplicateFinder.findByHash_ = function(hash, volumeId) {
Can we just inline this function where it is used thereby avoiding the need for
the duplicate method names?

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:103: return new
Promise(function(resolve, reject) {
Please wrap before anon functions and indent accordingly.

[Ben Kwa, 2015-01-28 18:33:28]

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:51: return new
Promise(function(resolve, reject) {
On 2015/01/28 18:17:40, Steve McKay wrote:
> Please wrap before anon functions...it is *very* hard to read when inlined.

Done.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:54:
function(result) {
On 2015/01/28 18:17:40, Steve McKay wrote:
> @param

Done.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:55: if
(!chrome.runtime.lastError) {
On 2015/01/28 18:17:40, Steve McKay wrote:
> The logic is unnecessarily twisted here.
> 
> if (chrome.runtime.lastError) {
>   reject(chrome.runtime.lastError);
> } else {
>   resolve(result);
> }

Done.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:80:
importer.DriveDuplicateFinder.prototype.getDriveId_ = function() {
On 2015/01/28 18:17:40, Steve McKay wrote:
> A comment intended to influence your future design decisions:
> 
> Drive ID is something that should be injected. That is obviously a pain given
> the arrangement with how this class is constructed. I think the second best
> thing would have been to inject a Promise.<string>.
> 
> Doing so would have kept the drive ID lookup logic separate from the core
dedupe
> logic and in doing so reduced the number of false failures that can happen on
> this test. It would also have made this class MUCH easier to test given
there'd
> be no need for a MockFileSystem at all.
> 
> Lastly it would eliminate that funky null promise that is lazily initialized.

My take on this: the fact that the search code requires a volume ID is an
implementation detail that clients shouldn't have to worry about.  The
MockFileSystem actually isn't related to the volume ID at all - it's needed
purely to create MockFileEntries for testing.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:102:
importer.DriveDuplicateFinder.findByHash_ = function(hash, volumeId) {
On 2015/01/28 18:17:40, Steve McKay wrote:
> Can we just inline this function where it is used thereby avoiding the need
for
> the duplicate method names?

That's how I originally had it, and I thought breaking it out into a separate
function made it a lot more readable.

https://codereview.chromium.org/885653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:103: return new
Promise(function(resolve, reject) {
On 2015/01/28 18:17:40, Steve McKay wrote:
> Please wrap before anon functions and indent accordingly.

Done.

[Ben Kwa, 2015-01-28 18:41:50]

I renamed the internal function so they're not both called 'findByHash_'.

[mtomasz, 2015-01-28 23:00:37]

lgtm with nits

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

https://codereview.chromium.org/885653003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:39: /** @param
{!Array<string>} urls */
nit: @return missing.

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

https://codereview.chromium.org/885653003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:100:
reportPromise(
nit: indent wrong

[Ben Kwa, 2015-01-28 23:13:55]

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

https://codereview.chromium.org/885653003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:39: /** @param
{!Array<string>} urls */
On 2015/01/28 23:00:37, mtomasz wrote:
> nit: @return missing.

Done.

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

https://codereview.chromium.org/885653003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:100:
reportPromise(
On 2015/01/28 23:00:37, mtomasz wrote:
> nit: indent wrong

Done.

[Ben Kwa, 2015-01-28 23:14:00]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-01-28 23:15:00]

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

[commit-bot: I haz the power, 2015-01-28 23:50:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-28 23:51:09]

Patchset 4 (id:??) landed as
https://crrev.com/3244b994fd8c52b595b3bc101a7e1ed77b04c0f0
Cr-Commit-Position: refs/heads/master@{#313621}