Improved security of Quick View by rendering videos and audios inside webview.

Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/efc63a360e385ffc9f7c72d47e0aef618b594e6c
Cr-Commit-Position: refs/heads/master@{#411004}

[oka, 2016-07-26 12:06:11]

nit

[oka, 2016-07-26 12:23:21]

nit

[oka, 2016-07-26 12:26:45]

oka@chromium.org changed reviewers:
+ fukino@chromium.org

[oka, 2016-07-26 12:26:46]

PTAL. This CL set upstream to https://codereview.chromium.org/2181953003/.

[oka, 2016-07-26 12:28:40]

Correction: set upstream to https://codereview.chromium.org/2140113003/.

[fukino, 2016-07-27 07:36:06]

Is it possible to combine the files for image, video, and audio? The
implementation looks similar, so combining them will work doing to avoid
unnecessary imports and reduce the size (and easier maintenance).

Also, if there is no UI tests for quick view yet, could you add some? (It's OK
to have It in a separate CL)

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:234:
reject();
It should be better to keep the error object from entry.file(), like:

entry.file(resolve, function(error) {
  console.error('Failed to get file.');
  reject(error);
});

Or, simply

entry.file(resolve, reject);

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:287: var /**
@type {!Array<!MetadataItem>} */ items = values[0];
This annotation looks unusual.

It may be

var items = /** @type {!Array<!MetadataItem>} */ (values[0]);

or

/** @type {!Array<!MetadataItem>} */
var items = values[0];

?

[oka, 2016-07-27 08:44:41]

rebase.

[oka, 2016-07-27 09:32:48]

Address comments.

[oka, 2016-07-27 10:30:15]

Add files-safe-media element which unifies files-safe-{img,video,audio}.

[oka, 2016-07-27 10:41:47]

nit

[oka, 2016-07-27 10:42:08]

Description was changed from

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-video and files-safe-audio tag, which renders contents
inside webview, and used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and
used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[oka, 2016-07-27 10:45:54]

nit

[oka, 2016-07-27 10:46:18]

PTAL.

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:234:
reject();
On 2016/07/27 07:36:06, fukino wrote:
> It should be better to keep the error object from entry.file(), like:
> 
> entry.file(resolve, function(error) {
>   console.error('Failed to get file.');
>   reject(error);
> });
> 
> Or, simply
> 
> entry.file(resolve, reject);

Done.

https://codereview.chromium.org/2181953003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:287: var /**
@type {!Array<!MetadataItem>} */ items = values[0];
On 2016/07/27 07:36:06, fukino wrote:
> This annotation looks unusual.
> 
> It may be
> 
> var items = /** @type {!Array<!MetadataItem>} */ (values[0]);
> 
> or
> 
> /** @type {!Array<!MetadataItem>} */
> var items = values[0];
> 
> ?

Done.

[fukino, 2016-07-28 12:31:38]

lgtm with nits.

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:248:
params.contentUrl = URL.createObjectURL(file);
nit: Should be 2 tab.

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:270: return
params;
ditto

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:282: /**
@type {!Array<!MetadataItem>} */
ditto

[oka, 2016-08-02 00:19:20]

Address comments.

[oka, 2016-08-02 00:24:00]

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:248:
params.contentUrl = URL.createObjectURL(file);
On 2016/07/28 12:31:38, fukino wrote:
> nit: Should be 2 tab.

Done.

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:270: return
params;
On 2016/07/28 12:31:38, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2181953003/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:282: /**
@type {!Array<!MetadataItem>} */
On 2016/07/28 12:31:38, fukino wrote:
> ditto

Done.

[oka, 2016-08-04 07:15:08]

Rebase.

[oka, 2016-08-04 07:31:38]

The CQ bit was checked by oka@chromium.org

[oka, 2016-08-04 07:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2181953003/#ps260001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-08-04 07:32:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 08:26:58]

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

[commit-bot: I haz the power, 2016-08-04 08:26:59]

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

[oka, 2016-08-04 09:33:39]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2016-08-04 09:34:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 10:31:21]

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

[commit-bot: I haz the power, 2016-08-04 10:31:22]

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

[oka, 2016-08-10 07:31:38]

The CQ bit was checked by oka@chromium.org

[oka, 2016-08-10 07:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2181953003/#ps300001
(title: "Nit")

[commit-bot: I haz the power, 2016-08-10 07:32:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 08:27:31]

Description was changed from

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and
used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and
used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[commit-bot: I haz the power, 2016-08-10 08:27:33]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-08-10 08:28:54]

Description was changed from

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and
used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
Improved security of Quick View by rendering videos and audios inside webview.

Added files-safe-media tag which generalizes and replaces files-safe-img, and
used them in Quick View.
This addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21.

BUG=614228,627698
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/efc63a360e385ffc9f7c72d47e0aef618b594e6c
Cr-Commit-Position: refs/heads/master@{#411004}
==========

[commit-bot: I haz the power, 2016-08-10 08:28:56]

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