Remove passing WebString across threads

Remove passing WebString across threads

Create bridge functions that converts arguments into Blink objects
(such as WebFileSystemEntry and WebFileInfo) after crossing threads
and then call WebFileSystemCallbacks methods.

BUG=393900
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283627

[tzik, 2014-07-15 10:41:27]

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:132: class
WebFileSystemCallbacksBridge {
I think it's not worth adding new class to combine these method.
Can we add functions separately for each method of the class?
Such as:
void didOpenFilesystem(const base::string16& name, const GURL& root,
WebFileSystemCallbacks* callbacks);

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:189: void RunCallbacks(int
callbacks_id, Method method, const Params& params,
(continued from above)
And replace Method and Params with a
base::Callback<void(WebFileSystemCallbacks*)>?

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:243: CallbackFileSystemCallbacks(
(continued from above)
And then, could you replace CallbackFilesystemCallbacks with
DispatchResultClosure + base::Bind?

[hiroshige, 2014-07-15 12:30:31]

Thank you for reviewing and suggestions!

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:132: class
WebFileSystemCallbacksBridge {
On 2014/07/15 10:41:26, tzik wrote:
> I think it's not worth adding new class to combine these method.
> Can we add functions separately for each method of the class?
> Such as:
> void didOpenFilesystem(const base::string16& name, const GURL& root,
> WebFileSystemCallbacks* callbacks);

Done.

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:189: void RunCallbacks(int
callbacks_id, Method method, const Params& params,
On 2014/07/15 10:41:26, tzik wrote:
> (continued from above)
> And replace Method and Params with a
> base::Callback<void(WebFileSystemCallbacks*)>?

Done.

https://codereview.chromium.org/391083002/diff/1/content/child/fileapi/webfil...
content/child/fileapi/webfilesystem_impl.cc:243: CallbackFileSystemCallbacks(
I retained CallbackFilesystemCallbacks to make the code diff smaller.

[tzik, 2014-07-15 13:41:06]

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:132: void
didSucceed(WebFileSystemCallbacks *callbacks) {
s/didSucceed/DidSucceed/, s/ *callbacks/* callbacks/ to follow Chromium C++
style.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:137: WebFileSystemCallbacks
*callbacks) {
ditto

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:144: bool has_more,
WebFileSystemCallbacks *callbacks) {
ditto

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:146: for (size_t i = 0; i <
entries.size(); i++) {
s/i++/++i/?

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:155: WebFileSystemCallbacks
*callbacks) {
ditto

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:158: static_cast<WebURL>(root));
Can we remove static_cast and let implicit conversion work?

[hiroshige, 2014-07-16 04:09:16]

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:132: void
didSucceed(WebFileSystemCallbacks *callbacks) {
On 2014/07/15 13:41:06, tzik wrote:
> s/didSucceed/DidSucceed/, s/ *callbacks/* callbacks/ to follow Chromium C++
> style.

Done.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:137: WebFileSystemCallbacks
*callbacks) {
On 2014/07/15 13:41:06, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:144: bool has_more,
WebFileSystemCallbacks *callbacks) {
On 2014/07/15 13:41:06, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:146: for (size_t i = 0; i <
entries.size(); i++) {
On 2014/07/15 13:41:06, tzik wrote:
> s/i++/++i/?

Done.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:155: WebFileSystemCallbacks
*callbacks) {
On 2014/07/15 13:41:06, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/391083002/diff/20001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:158: static_cast<WebURL>(root));
Done, except for mount_type (explicit casting is needed).

[tzik, 2014-07-16 04:22:51]

LGTM!

https://codereview.chromium.org/391083002/diff/40001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/40001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:158: root);
can these arguments be packed to l156?

[hiroshige, 2014-07-16 04:31:34]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2014-07-16 04:31:50]

Thanks!

https://codereview.chromium.org/391083002/diff/40001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/391083002/diff/40001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:158: root);
On 2014/07/16 04:22:51, tzik wrote:
> can these arguments be packed to l156?

Done.

[commit-bot: I haz the power, 2014-07-16 04:34:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hiroshige@chromium.org/391083002/60001

[tzik, 2014-07-16 04:43:38]

On 2014/07/16 04:34:37, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/hiroshige@chromium.org/391083002/60001

Aw.
Adding Michael. Could you double check the CL as a owner?

[hiroshige, 2014-07-16 04:45:59]

The CQ bit was unchecked by hiroshige@chromium.org

[michaeln, 2014-07-16 19:16:00]

r/s lgtm2, nice catch

[hiroshige, 2014-07-17 03:40:18]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:42:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hiroshige@chromium.org/391083002/60001

[commit-bot: I haz the power, 2014-07-17 03:50:02]

Change committed as 283627

[kinuko, 2014-07-21 13:38:50]

Thx for fixing this.  Btw would it be possible/practical to have some thread
assertions in WebString not to overlook these patterns?