Files.app: Tag cloud imported media files.

Files.app: Tag cloud imported media files.

Tags media files that were uploaded via cloud import with a custom Drive property.

BUG=469774
Committed: https://crrev.com/57c88499fd6343e84e92f93e4d2811df4d8aed00
Cr-Commit-Position: refs/heads/master@{#321853}

[Ben Kwa, 2015-03-23 17:12:15]

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

[Steve McKay, 2015-03-23 18:49:48]

LGTM w/ nits.

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:148: var
UpdateType = importer.MediaImportHandler.ImportTask.UpdateType;
The alias isn't worth the win (which is VERY small in this case). Just:

if (updateType !==
    importer.MediaImportHandler.ImportTask.UpdateType) {
  return;
}

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:154: // @type
{!importer.MediaImportHandler.ImportTask.EntryChangedInfo}
I'm pretty sure closure doesn't understand this comment. Even if this is just
for reader's benefit, I'd just make this a regular closure comment.


/** @type {!importer.MediaImportHandler.ImportTask.EntryChangedInfo} */

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:159:
console.error('setEntryTag error: ' + chrome.runtime.lastError.message);
'Unable to tag imported media: ' + ....

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:166:
chrome.fileManagerPrivate.setEntryTag(
Wrap all args, add a comment to each (where useful).


chrome.fileManagerPrivate.setEntryTag(
    info.destination.toURL(),
    'private',  // scoped to just this app
    'cloud-import',  // a magic string, maybe put it in a constant so the
variable name provides more meaning.
    'media', // I dunno what this is, needs a comment.
    onError  // why not just inline this function?
);

[Ben Kwa, 2015-03-23 20:45:46]

The CQ bit was checked by kenobi@chromium.org

[Ben Kwa, 2015-03-23 20:45:47]

The patchset sent to the CQ was uploaded after l-g-t-m from smckay@chromium.org
Link to the patchset: https://codereview.chromium.org/1032513002/#ps20001
(title: "Address feedback.")

[commit-bot: I haz the power, 2015-03-23 20:47:04]

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

[commit-bot: I haz the power, 2015-03-23 21:16:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-23 21:17:43]

Patchset 2 (id:??) landed as
https://crrev.com/57c88499fd6343e84e92f93e4d2811df4d8aed00
Cr-Commit-Position: refs/heads/master@{#321853}

[Ben Kwa, 2015-03-23 21:36:26]

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:148: var
UpdateType = importer.MediaImportHandler.ImportTask.UpdateType;
On 2015/03/23 18:49:48, Steve McKay wrote:
> The alias isn't worth the win (which is VERY small in this case). Just:
> 
> if (updateType !==
>     importer.MediaImportHandler.ImportTask.UpdateType) {
>   return;
> }

Done.

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:154: // @type
{!importer.MediaImportHandler.ImportTask.EntryChangedInfo}
On 2015/03/23 18:49:48, Steve McKay wrote:
> I'm pretty sure closure doesn't understand this comment. Even if this is just
> for reader's benefit, I'd just make this a regular closure comment.
> 
> 
> /** @type {!importer.MediaImportHandler.ImportTask.EntryChangedInfo} */

Oops - yes, that was meant to be a closure comment.  Done.

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:159:
console.error('setEntryTag error: ' + chrome.runtime.lastError.message);
On 2015/03/23 18:49:48, Steve McKay wrote:
> 'Unable to tag imported media: ' + ....

Done.

https://codereview.chromium.org/1032513002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/media_import_handler.js:166:
chrome.fileManagerPrivate.setEntryTag(
On 2015/03/23 18:49:48, Steve McKay wrote:
> Wrap all args, add a comment to each (where useful).
> 
> 
> chrome.fileManagerPrivate.setEntryTag(
>     info.destination.toURL(),
>     'private',  // scoped to just this app
>     'cloud-import',  // a magic string, maybe put it in a constant so the
> variable name provides more meaning.
>     'media', // I dunno what this is, needs a comment.
>     onError  // why not just inline this function?
> );

Done.