Fix webkitGetEntry for non-native files.

Fix webkitGetEntry for non-native files.

Previously webkitGetEntry was broken for non-native files. The method
creates FileEntry in isolated file system from DataTransferItem. The
problem is DataTransferItem only has file system ID of isolated file
system containing native files, though the browser process prepares
different isolated file system for non-native files.

The CL adds new field 'fileSystemId' for types representing a non-native
file and let webkitGetEntry refer the fileSystemId so that it can return
valid FileEntry for non-native files.

BUG=671811
TEST=Drop a file from Chrome OS Files app to Google Drive web.

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

[hirono, 2016-12-12 04:14:53]

The CQ bit was checked by hirono@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 04:15:04]

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

[hirono, 2016-12-12 04:20:26]

The CQ bit was checked by hirono@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 04:20:32]

Dry run: 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-12-12 04:33:21]

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

[commit-bot: I haz the power, 2016-12-12 04:33:22]

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

[hirono, 2016-12-12 04:45:10]

The CQ bit was checked by hirono@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 04:45:29]

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

[hirono, 2016-12-12 04:54:05]

hirono@chromium.org changed reviewers:
+ hashimoto@chromium.org

[hirono, 2016-12-12 04:54:06]

hashimoto@-san, could you take a look at this? Thank you!

[hashimoto, 2016-12-12 05:55:38]

hashimoto@chromium.org changed reviewers:
+ kinuko@chromium.org

[hashimoto, 2016-12-12 05:55:39]

+kinuko@ should be a better reviewer.

I have touched WebKit code only a few times, a number of years ago, and I have
no ownership here.

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
File content/public/common/drop_data.h (right):

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
content/public/common/drop_data.h:34: base::string16 filesystem_id;
IIUC a filesystem ID is an always ASCII string.
Why does this need to be a string16?

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectTest.cpp (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectTest.cpp:25:
m_dataObject->addFilename(filePath, String(), "fileSystemId");
Instead of adding filesystem ID to all existing tests, how about adding a new
test case for filesystem ID handling?

[satorux1, 2016-12-12 06:24:01]

satorux@chromium.org changed reviewers:
+ satorux@chromium.org, tzik@chromium.org

[satorux1, 2016-12-12 06:24:01]

+tzik as kinuko may not be able to take a look.

[commit-bot: I haz the power, 2016-12-12 06:24:43]

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

[commit-bot: I haz the power, 2016-12-12 06:24:44]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-12-12 08:03:35]

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
File content/public/common/drop_data.h (right):

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
content/public/common/drop_data.h:34: base::string16 filesystem_id;
On 2016/12/12 05:55:39, hashimoto wrote:
> IIUC a filesystem ID is an always ASCII string.
> Why does this need to be a string16?

Looks like just using string is fine.  We can use WebString::ascii and
WebString::fromASCII for conversions
(For context: historically we often tended to use string16 for the strings that
are passed to blink because WebString has implicit conversion to/from string16,
but we're considering removing the implicit conversion)

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObject.cpp:304: if
(!data.filesystemId().isNull()) {
nit: it looks we can just always call setFilesystemId

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectItem.h (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectItem.h:53: static
DataObjectItem* createFromFileEntry(File*, const String& fileSystemId);
I would expect that it takes FileEntry for this method name. 
createFromFileInFileSystem or createFromFileWithFilesystemId?

[hirono, 2016-12-13 08:10:25]

The CQ bit was checked by hirono@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 08:10:45]

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

[hirono, 2016-12-13 08:12:13]

Thank you!

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
File content/public/common/drop_data.h (right):

https://codereview.chromium.org/2565283002/diff/40001/content/public/common/d...
content/public/common/drop_data.h:34: base::string16 filesystem_id;
On 2016/12/12 08:03:35, kinuko wrote:
> On 2016/12/12 05:55:39, hashimoto wrote:
> > IIUC a filesystem ID is an always ASCII string.
> > Why does this need to be a string16?
> 
> Looks like just using string is fine.  We can use WebString::ascii and
> WebString::fromASCII for conversions
> (For context: historically we often tended to use string16 for the strings
that
> are passed to blink because WebString has implicit conversion to/from
string16,
> but we're considering removing the implicit conversion)

Thank you for the explanation. Done!

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObject.cpp:304: if
(!data.filesystemId().isNull()) {
On 2016/12/12 08:03:35, kinuko wrote:
> nit: it looks we can just always call setFilesystemId

Done.

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectItem.h (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectItem.h:53: static
DataObjectItem* createFromFileEntry(File*, const String& fileSystemId);
On 2016/12/12 08:03:35, kinuko wrote:
> I would expect that it takes FileEntry for this method name. 
> createFromFileInFileSystem or createFromFileWithFilesystemId?

Done.

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectTest.cpp (right):

https://codereview.chromium.org/2565283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectTest.cpp:25:
m_dataObject->addFilename(filePath, String(), "fileSystemId");
On 2016/12/12 05:55:39, hashimoto wrote:
> Instead of adding filesystem ID to all existing tests, how about adding a new
> test case for filesystem ID handling?

Done.

[commit-bot: I haz the power, 2016-12-13 10:15:19]

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

[commit-bot: I haz the power, 2016-12-13 10:15:20]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-12-13 14:45:10]

lgtm

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObject.cpp:308: }
nit: no need of { } for single-line body)

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectItem.h (right):

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectItem.h:54: //
webkitGetAsEntry.
filesystem ID -> non-empty filesystem ID ?

[hirono, 2016-12-14 04:54:55]

Thank you!

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObject.cpp:308: }
On 2016/12/13 14:45:10, kinuko wrote:
> nit: no need of { } for single-line body)

Done.

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectItem.h (right):

https://codereview.chromium.org/2565283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectItem.h:54: //
webkitGetAsEntry.
On 2016/12/13 14:45:10, kinuko wrote:
> filesystem ID -> non-empty filesystem ID ?

Done.

[hirono, 2016-12-14 04:54:58]

The CQ bit was checked by hirono@chromium.org

[hirono, 2016-12-14 04:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2565283002/#ps80001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-12-14 04:55:20]

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-12-14 04:57:05]

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

[commit-bot: I haz the power, 2016-12-14 04:57:07]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hirono, 2016-12-14 05:42:38]

The CQ bit was checked by hirono@chromium.org

[hirono, 2016-12-14 05:42:39]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2565283002/#ps100001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-12-14 05:42:50]

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-12-14 07:36:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481694158780270,
"parent_rev": "d03096139380aad559603220365ff3ec51c269fc", "commit_rev":
"64ca1231c298ec097e0a398382d5184ee734e7b1"}

[commit-bot: I haz the power, 2016-12-14 07:36:50]

Description was changed from

==========
Fix webkitGetEntry for non-native files.

Previously webkitGetEntry was broken for non-native files. The method
creates FileEntry in isolated file system from DataTransferItem. The
problem is DataTransferItem only has file system ID of isolated file
system containing native files, though the browser process prepares
different isolated file system for non-native files.

The CL adds new field 'fileSystemId' for types representing a non-native
file and let webkitGetEntry refer the fileSystemId so that it can return
valid FileEntry for non-native files.

BUG=671811
TEST=Drop a file from Chrome OS Files app to Google Drive web.
==========

to

==========
Fix webkitGetEntry for non-native files.

Previously webkitGetEntry was broken for non-native files. The method
creates FileEntry in isolated file system from DataTransferItem. The
problem is DataTransferItem only has file system ID of isolated file
system containing native files, though the browser process prepares
different isolated file system for non-native files.

The CL adds new field 'fileSystemId' for types representing a non-native
file and let webkitGetEntry refer the fileSystemId so that it can return
valid FileEntry for non-native files.

BUG=671811
TEST=Drop a file from Chrome OS Files app to Google Drive web.

Review-Url: https://codereview.chromium.org/2565283002
==========

[commit-bot: I haz the power, 2016-12-14 07:36:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-14 07:38:47]

Description was changed from

==========
Fix webkitGetEntry for non-native files.

Previously webkitGetEntry was broken for non-native files. The method
creates FileEntry in isolated file system from DataTransferItem. The
problem is DataTransferItem only has file system ID of isolated file
system containing native files, though the browser process prepares
different isolated file system for non-native files.

The CL adds new field 'fileSystemId' for types representing a non-native
file and let webkitGetEntry refer the fileSystemId so that it can return
valid FileEntry for non-native files.

BUG=671811
TEST=Drop a file from Chrome OS Files app to Google Drive web.

Review-Url: https://codereview.chromium.org/2565283002
==========

to

==========
Fix webkitGetEntry for non-native files.

Previously webkitGetEntry was broken for non-native files. The method
creates FileEntry in isolated file system from DataTransferItem. The
problem is DataTransferItem only has file system ID of isolated file
system containing native files, though the browser process prepares
different isolated file system for non-native files.

The CL adds new field 'fileSystemId' for types representing a non-native
file and let webkitGetEntry refer the fileSystemId so that it can return
valid FileEntry for non-native files.

BUG=671811
TEST=Drop a file from Chrome OS Files app to Google Drive web.

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

[commit-bot: I haz the power, 2016-12-14 07:38:48]

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