Rework update model to eliminate a "flicker" resulting from the brief update to zero results when a…

1) Add a drop down revealing a popup panel with import details (with is a WIP, but the current state should be sufficient to land).

2) Update scanner to publish updates on all scan activity (so the details reflects valid information while scanning). In doing this I pulled all of the deduping logic up out of ScanResult into MediaScanner...improving separation of concern between the classes.

3) Rework update model to eliminate a flicker resulting from the brief update to show zero results when a file selection is transitioning from n to n+/-1.

Sadly, this change results in a net loss of readability, especially WRT having good clear decision points. I've added comments to help out in this respect.

FYI, I'm OOO for the rest of the day. I think this CL is good to land, rough though it may be.

In a followup CL:

1) I'll make some subsequent improvements to unittests in the wake of removing the "GetCommandUpdate" coverage (removed because GetCommandUpdate exists no more).

2) This change still exhibits one odd behavior, ignoring entirely a selection when it consistes of all deduped files...in that case we *do* want to see zero results. Will address at the time of test improvements.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/873d9c13d08de9449b659fd5dad1fcf579758a3a
Cr-Commit-Position: refs/heads/master@{#315094}

[Steve McKay, 2015-02-04 18:43:27]

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

[hirono, 2015-02-05 10:51:43]

lgtm!

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:233:
console.log('Skipping file entry...watched directory was modified.');
Is it intentionally left here?

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:363: * Attempt
to scan the current context.
nit: Attempts

[Ben Kwa, 2015-02-05 14:54:27]

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:205:
console.assert(entry.isFile);
Suggestion: include an explanatory string in the assert.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:239: // funciton
findEntriesRecursively does that. So we
function

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:264: return
Promise.resolve(false);
I think this should just be Promise.resolve().  The boolean return means nothing
here.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:266: return
this.hasHistoryDuplicate_(entry).then(
nit: line break before .then

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:282:
}.bind(this)).then(
nit: line break before .then

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:441: // We wait to
check the hashcode until after all
I find this comment confusing.  Which async data are we waiting for?

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/css/file_manager.css:329: height: 300px;
alpha sort these

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/css/file_manager.css:347: padding: 10px;
alpha sort these

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:366: *    
actively scanning, finished or in any other state.
"A scan object, or null if scan is not possible in current context".

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:754:
importer.ScanManager.prototype.getSelectionScan = function(entries) {
If this function is called with a set of entries, but the selection scan isn't
currently null, the function returns the current scan and just throws away the
entries it was passed.  This seems like unintuitive behaviour to me.

[Steve McKay, 2015-02-05 15:46:29]

New patchsets have been uploaded after l-g-t-m from hirono@chromium.org

[Steve McKay, 2015-02-05 15:46:31]

Respond to review comments.

[Steve McKay, 2015-02-05 15:46:47]

Dones. PTAL.

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:205:
console.assert(entry.isFile);
On 2015/02/05 14:54:26, Ben Kwa wrote:
> Suggestion: include an explanatory string in the assert.

Removed.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:233:
console.log('Skipping file entry...watched directory was modified.');
On 2015/02/05 10:51:43, hirono wrote:
> Is it intentionally left here?

It was, but on second thought...removed it.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:239: // funciton
findEntriesRecursively does that. So we
On 2015/02/05 14:54:26, Ben Kwa wrote:
> function

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:264: return
Promise.resolve(false);
On 2015/02/05 14:54:26, Ben Kwa wrote:
> I think this should just be Promise.resolve().  The boolean return means
nothing
> here.

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:266: return
this.hasHistoryDuplicate_(entry).then(
On 2015/02/05 14:54:26, Ben Kwa wrote:
> nit: line break before .then

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:282:
}.bind(this)).then(
On 2015/02/05 14:54:26, Ben Kwa wrote:
> nit: line break before .then

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:441: // We wait to
check the hashcode until after all
On 2015/02/05 14:54:26, Ben Kwa wrote:
> I find this comment confusing.  Which async data are we waiting for?

Rewrote to make the thought clearer.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/css/file_manager.css:329: height: 300px;
On 2015/02/05 14:54:26, Ben Kwa wrote:
> alpha sort these

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/css/file_manager.css:347: padding: 10px;
On 2015/02/05 14:54:27, Ben Kwa wrote:
> alpha sort these

Done.

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

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:363: * Attempt
to scan the current context.
On 2015/02/05 10:51:43, hirono wrote:
> nit: Attempts

Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:366: *    
actively scanning, finished or in any other state.
On 2015/02/05 14:54:27, Ben Kwa wrote:
> "A scan object, or null if scan is not possible in current context".

More better. Done.

https://codereview.chromium.org/899943002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:754:
importer.ScanManager.prototype.getSelectionScan = function(entries) {
On 2015/02/05 14:54:27, Ben Kwa wrote:
> If this function is called with a set of entries, but the selection scan isn't
> currently null, the function returns the current scan and just throws away the
> entries it was passed.  This seems like unintuitive behaviour to me.

I agree. The logic in this class has gotten convoluted...deriving from our
specialized selection scan cases. In any case, I think that was a
relic...converted to an assert.

[Ben Kwa, 2015-02-05 15:51:13]

lgtm

[Steve McKay, 2015-02-05 19:53:02]

New patchsets have been uploaded after l-g-t-m from kenobi@chromium.org

[Steve McKay, 2015-02-05 20:12:33]

Update tests to work with new update model (where we don't update immediately
when creating a scan).

[Steve McKay, 2015-02-05 20:12:36]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:13:14]

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

[commit-bot: I haz the power, 2015-02-05 20:16:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:16:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steve McKay, 2015-02-05 21:39:18]

Merge in Fukino-sans changes.

[Steve McKay, 2015-02-05 21:39:21]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-02-05 21:39:39]

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

[commit-bot: I haz the power, 2015-02-05 22:50:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 22:50:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steve McKay, 2015-02-05 23:26:53]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:27:30]

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

[commit-bot: I haz the power, 2015-02-06 00:31:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-06 00:31:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steve McKay, 2015-02-06 19:17:23]

Don't use drive icon in dialog for now, since this breaks funcitonal tests.

[Steve McKay, 2015-02-06 19:17:31]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-02-06 19:17:56]

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

[Steve McKay, 2015-02-06 19:30:14]

FYI, FileManagerTests were broken because the test harness for functional tests
uses "volume-type-icon=drive" to qualify the volume label...when trying to find
a thing to click.

Since we added another DOM element with "volume-type-icon=drive"...tests broke.

My change works around this problem by using an lame icon. In a followup CL we
can further qualify the query to ensure it accurately selects the correct label
for clicking.

Submitting.

[commit-bot: I haz the power, 2015-02-06 20:07:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-06 20:08:20]

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