Issue 865343002: Files.app: Invalidate ScanResult when the scanned directory is changed.

Issue 865343002: Files.app: Invalidate ScanResult when the scanned directory is changed. (Closed)

Created:
5 years, 11 months ago by hirono

Modified:
5 years, 11 months ago

Reviewers:
Steve McKay

CC:
chromium-reviews, tfarina, rginda+watch_chromium.org, mtomasz+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Files.app: Invalidate ScanResult when the scanned directory is changed. * Add whenInvalidated method to ScanResult. * Watch the invalidation in ImportController. BUG=420680 TEST=FileManagerJsTest.MediaScannerTest Committed: https://crrev.com/9ec42b887a697f656b4df721dbde5cd08104af96 Cr-Commit-Position: refs/heads/master@{#312977}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 16

Patch Set 5 : #

Total comments: 16

Patch Set 6 : Update test. #

Patch Set 7 : Fix closure compiler error. #

Patch Set 8 : #

Patch Set 9 : #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+218 lines, -33 lines)			Patch
M	ui/file_manager/file_manager/background/js/background.js	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download
M	ui/file_manager/file_manager/background/js/media_scanner.js	View	1 2 3 4 5 6 7 8	9 chunks	+144 lines, -24 lines	0 comments	Download
M	ui/file_manager/file_manager/background/js/media_scanner_unittest.html	View	1 2 3 4 5	1 chunk	+3 lines, -2 lines	0 comments	Download
M	ui/file_manager/file_manager/background/js/media_scanner_unittest.js	View	1 2 3 4 5 6 7	3 chunks	+27 lines, -1 line	0 comments	Download
M	ui/file_manager/file_manager/background/js/mock_media_scanner.js	View	1 2 3 4 5	1 chunk	+28 lines, -0 lines	0 comments	Download
M	ui/file_manager/file_manager/common/js/importer_common.js	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/import_controller.js	View	1 2 3 4 5 6 7	4 chunks	+12 lines, -4 lines	0 comments	Download

Messages

Total messages: 11 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Steve McKay

I like the new direction. https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_manager/background/js/media_scanner.js File ui/file_manager/file_manager/background/js/media_scanner.js (right): https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_manager/background/js/media_scanner.js#newcode9 ui/file_manager/file_manager/background/js/media_scanner.js:9: importer.DirectoryWatcher = function(callback) { ...

5 years, 11 months ago (2015-01-23 20:02:29 UTC) #3

hirono

Thanks. I'll update tests also soon. https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_manager/background/js/media_scanner.js File ui/file_manager/file_manager/background/js/media_scanner.js (right): https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_manager/background/js/media_scanner.js#newcode9 ui/file_manager/file_manager/background/js/media_scanner.js:9: importer.DirectoryWatcher = function(callback) ...

5 years, 11 months ago (2015-01-23 21:39:35 UTC) #4

Thanks. I'll update tests also soon.

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

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:9:
importer.DirectoryWatcher = function(callback) {
On 2015/01/23 20:02:28, Steve McKay wrote:
> Move this class to end of file. Keep more important interfaces and classes
near
> the top.

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:30:
importer.DirectoryWatcher.prototype.onDirectoryChanged_ = function(event) {
On 2015/01/23 20:02:29, Steve McKay wrote:
> onWatchedDirectoryModified_

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:44:
importer.DirectoryWatcherFactory = function() {
On 2015/01/23 20:02:28, Steve McKay wrote:
>   @typedef function(): !importer.DirectoryWatcher
> 
> Then you can just add a "create" method to DirectoryWatcher.
> 
>   importer.DirectoryWatcher.create = function(callback) {
>     return new importer.DirectoryWatcher(callback);
>   };
> 
> And pass that function into DefaultMediaScanner.
> 
> e.g.
> 
>   new DefaultMediaScanner(DirectoryWatcher.create);

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:65: * @param
{boolean} needWatch Whether to need to watch the scanned directories.
On 2015/01/23 20:02:28, Steve McKay wrote:
> Do we still need this? If we're injecting a fake watcher in tests...?

Yes, this is not for test.
We don't watch directory for scan generated by selection.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:149: * @private
{function(boolean): !importer.DefaultScanResult}
On 2015/01/23 20:02:29, Steve McKay wrote:
> COnvert this JS doc to use method style:
> 
> 
> /**
>  * @param {boolean} ...
>  * @return {!importer.DefaultScanResult}
>  */

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:187: 
On 2015/01/23 20:02:29, Steve McKay wrote:
> We should instantiate the watcher and handle invalidation here in the same
scope
> as the ScanResult.

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:250:
result.invalidated = true;
On 2015/01/23 20:02:29, Steve McKay wrote:
> Don't poke at a naked field. Add a new method "invalidateScan"

Done.

https://codereview.chromium.org/865343002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:311:
this.watchedDirectoryRoot = watchedDirectoryRoot;
On 2015/01/23 20:02:29, Steve McKay wrote:
> We can drop this now, correct?

No, this is referred from ImportController to check if the invalidated scan is
current directory's one or not.

Steve McKay

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_manager/background/js/media_scanner.js File ui/file_manager/file_manager/background/js/media_scanner.js (right): https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_manager/background/js/media_scanner.js#newcode16 ui/file_manager/file_manager/background/js/media_scanner.js:16: * @param {boolean} needWatch Whether to need to watch ...

5 years, 11 months ago (2015-01-23 22:26:10 UTC) #5

Steve McKay

I know you'll be traveling soon, so I'll prospectively LGTM this. If we need followup ...

5 years, 11 months ago (2015-01-23 22:28:05 UTC) #6

hirono

Thanks! https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_manager/background/js/media_scanner.js File ui/file_manager/file_manager/background/js/media_scanner.js (right): https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_manager/background/js/media_scanner.js#newcode16 ui/file_manager/file_manager/background/js/media_scanner.js:16: * @param {boolean} needWatch Whether to need to ...

5 years, 11 months ago (2015-01-23 23:36:46 UTC) #7

Thanks!

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

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:16: * @param
{boolean} needWatch Whether to need to watch the scanned directories.
On 2015/01/23 22:26:10, Steve McKay wrote:
> When would we NOT want to watch scanned directories?
> 
> FYI: Those who have worked with me for a while know that I am highly allergic
to
> boolean "behave this way arguments". They are a code smell that the code isn't
> factored the right way.
> 
> I don't think we need this arg at all. Since in any case we scan a directory,
> we'll want to watch it.

Done.

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:94: * @param
{!importer.DirectoryWatcherFactory=} opt_watcherFactory
On 2015/01/23 22:26:10, Steve McKay wrote:
> Avoid opt args. It's easy enough for someone to pass:\
> 
> importer.DirectoryWatcher.create

Done.

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:142: var scanResult
= this.createScanResult_(
On 2015/01/23 22:26:10, Steve McKay wrote:
> All of this conditional code is not palatable. Ideally, with sufficient
> dependency injection, we should need no conditions and no optional arguments.

Done.

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:149:
observer(importer.ScanEvent.FINALIZED, scanResult);
On 2015/01/23 22:26:10, Steve McKay wrote:
> INVALIDATED

Done.

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:492: * @typedef
{function(function()): !importer.DirectoryWatcher}
On 2015/01/23 22:26:10, Steve McKay wrote:
> Typedef the argument to the first function.

Done.

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

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:27:
VolumeManagerCommon.VolumeType.DOWNLOADS
On 2015/01/23 22:26:10, Steve McKay wrote:
> Remove this line.

Done.

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

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:75: if ((event
=== importer.ScanEvent.FINALIZED ||
On 2015/01/23 22:26:10, Steve McKay wrote:
> We need to remove the scan from the cachedScans_ object. Rather than our
> trolling through the object to find it (using "watchedDirectoryRoot") just
reset
> the entire chachedScans_ object.
> 
> this.cachedScans_ = {};
> 
> It's a blunt weapon, but multiple scans are a corner case.
> 
> NOTE: By removing it from the cache, then updating commands, the next time we
> try to look up a scan (when the command is updated), it won't be found > we'll
> start a new scan...completing the circle of life.

Done.

https://codereview.chromium.org/865343002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:242: if (!scan
|| scan.isInvalidated()) {
On 2015/01/23 22:26:10, Steve McKay wrote:
> Make the "isInvalidted" check an assert. As soon as the scan is invalidated we
> should remove it from the cache.

Done.

commit-bot: I haz the power

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

5 years, 11 months ago (2015-01-23 23:37:48 UTC) #9

commit-bot: I haz the power

5 years, 11 months ago (2015-01-24 00:28:29 UTC) #11

Message was sent while issue was closed.

Patchset 9 (id:??) landed as
https://crrev.com/9ec42b887a697f656b4df721dbde5cd08104af96
Cr-Commit-Position: refs/heads/master@{#312977}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages