Eliminate usage of InterfacePtr::WaitForIncomingResponse.

Eliminate usage of InterfacePtr::WaitForIncomingResponse.

This CL removes InterfacePtr::WaitForIncomingResponse definition
and replaces usage of them with either [Sync] call or
a nested MessageLoop waiting asynchronously.

BUG=622438
Committed: https://crrev.com/83055f715f72c2394efd11b6bb17656899f4c9d0
Cr-Commit-Position: refs/heads/master@{#404579}

[leonhsl(Using Gerrit), 2016-06-27 06:21:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 06:22:08]

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-06-27 06:55:00]

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

[commit-bot: I haz the power, 2016-06-27 06:55:02]

Dry run: 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_...)

[leonhsl(Using Gerrit), 2016-06-27 07:24:49]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 07:24:58]

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-06-27 08:08:28]

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

[commit-bot: I haz the power, 2016-06-27 08:08:29]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-06-27 11:03:15]

leon.han@intel.com changed reviewers:
+ erg@chromium.org, rockot@chromium.org, yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-06-27 11:03:16]

This CL is touching all callers of WaitForIncomingResponse except inside mojo/
folder, and I'd like to confirm several question firstly.

To eliminate usage of WaitForIncomingResponse in test codes, there are 2 option
solutions:
1,
Change the target mojo call to [Sync]. Just as the changes for filesystem in
this CL.
2,
Use a nested run loop to wait for the response asynchronously. Just as the
changes for components/leveldb/remote_iterator_unittest.cc in this CL.

Question 1: 
   Solution 1 is fine for filesystem?
Question 2: 
   I'm not sure which solution is OK for 
components/leveldb/leveldb_service_unittest.cc. WDYT?
Question 3: 
   Inline question as bellow, about how to detect pipe connection error without
WaitForIncomingResponse.

Thanks a lot!

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
File net/proxy/proxy_resolver_factory_mojo_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:273:
ASSERT_TRUE(client.encountered_error());
I think InterfacePtr would detect the connection error immediately once
interface impl binding got closed, but here the thread always got hung when I
ran the test case. 
Seems the connection error handler does not get called. Am I doing something
wrong here?

[Ken Rockot(use gerrit already), 2016-06-29 17:01:28]

rockot@chromium.org changed reviewers:
+ sky@chromium.org

[Ken Rockot(use gerrit already), 2016-06-29 17:01:29]

I think there's been a fundamental misunderstanding here. Most of these calls to
WaitFor* are here to make testing more convenient, not because a given message
inherently makes sense as a sync message.

Sync messages should be used sparingly, and none of these cases justify it as
far as I can tell except maybe CommandBuffer (someone else can speak to that,
+sky).

We do want to delete WaitForIncomingResponse/WaitForIncomingMethodCall, but it's
not as simple as finding all uses of it and marking the corresponding message as
sync. Most uses (tests) can be replaced with a spinning RunLoop waiting for an
async response.

WaitFor* methods actually block the calling thread (i.e. they don't even spin
the message loop), just like sync IPC does, and this is extremely heavy-handed
and very rarely necessary.

[yzshen1, 2016-06-29 21:36:20]

(sorry for late reply)

+1 for Ken's comment.

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
File net/proxy/proxy_resolver_factory_mojo_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:273:
ASSERT_TRUE(client.encountered_error());
On 2016/06/27 11:03:16, leonhsl wrote:
> I think InterfacePtr would detect the connection error immediately once
> interface impl binding got closed, but here the thread always got hung when I
> ran the test case. 
> Seems the connection error handler does not get called. Am I doing something
> wrong here?

Please look at base::MessageLoop::ScopedNestableTaskAllower
The code has multiple layers of base::RunLoop, and the connection error handler
won't be run on this layer if nestable task is not allowed, so it will wait
forever.

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:438: run_loop.RunUntilIdle();
Please note that RunUntilIdle is incorrect. The message loop is not guaranteed
to be busy while we are waiting for the client to disconnect.

[leonhsl(Using Gerrit), 2016-06-29 22:45:17]

On 2016/06/29 17:01:29, Ken Rockot wrote:
> I think there's been a fundamental misunderstanding here. Most of these calls
to
> WaitFor* are here to make testing more convenient, not because a given message
> inherently makes sense as a sync message.
> 
> Sync messages should be used sparingly, and none of these cases justify it as
> far as I can tell except maybe CommandBuffer (someone else can speak to that,
> +sky).
> 
> We do want to delete WaitForIncomingResponse/WaitForIncomingMethodCall, but
it's
> not as simple as finding all uses of it and marking the corresponding message
as
> sync. Most uses (tests) can be replaced with a spinning RunLoop waiting for an
> async response.
> 
> WaitFor* methods actually block the calling thread (i.e. they don't even spin
> the message loop), just like sync IPC does, and this is extremely heavy-handed
> and very rarely necessary.

Thanks Ken a lot for explanations. Yeah, I'm also not sure how to handle each
caller of WaitForIncomingResponse, to use [Sync] call or a nested RunLoop, so I
just created this CL from my understanding(my guess;-). As I found most of APIs
in filesystem are [Sync], so I darely also marked those tested by
WaitForIncomingResponse as [Sync]. But when leveldb came up, I'm really not sure
whether simply marking [Sync] is correct, so I just applied spinning RunLoop to
remote_iterator_unittest.cc, and raised up questions to confirm about
leveldb_service_unittest.cc which involves several leveldb mojom APIs to be
decided to be [Sync] or not.

[leonhsl(Using Gerrit), 2016-06-29 22:53:49]

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
File net/proxy/proxy_resolver_factory_mojo_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:273:
ASSERT_TRUE(client.encountered_error());
On 2016/06/29 21:36:20, yzshen1 wrote:
> On 2016/06/27 11:03:16, leonhsl wrote:
> > I think InterfacePtr would detect the connection error immediately once
> > interface impl binding got closed, but here the thread always got hung when
I
> > ran the test case. 
> > Seems the connection error handler does not get called. Am I doing something
> > wrong here?
> 
> Please look at base::MessageLoop::ScopedNestableTaskAllower
> The code has multiple layers of base::RunLoop, and the connection error
handler
> won't be run on this layer if nestable task is not allowed, so it will wait
> forever.
> 

Yeah I noticed the multiple layers of base::RunLoop but I did not go down
deeply. Thanks a lot for the hints! Now I know where should I dip down, firstly
I'll try to understand base::MessageLoop::ScopedNestableTaskAllower there.

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:438: run_loop.RunUntilIdle();
On 2016/06/29 21:36:20, yzshen1 wrote:
> Please note that RunUntilIdle is incorrect. The message loop is not guaranteed
> to be busy while we are waiting for the client to disconnect.

Understood.

[leonhsl(Using Gerrit), 2016-06-29 23:04:11]

Description was changed from

==========
Use sync call instead of InterfacePtr::WaitForIncomingResponse.

BUG=622438
==========

to

==========
Eliminate usage of InterfacePtr::WaitForIncomingResponse.

We'll replace them with either [Sync] call or a nested RunLoop waiting
asynchronously.

BUG=622438
==========

[leonhsl(Using Gerrit), 2016-06-29 23:05:15]

Sorry seems the cl description is too bad, changed.

[leonhsl(Using Gerrit), 2016-06-30 03:04:15]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 03:04:43]

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-06-30 04:44:16]

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

[commit-bot: I haz the power, 2016-06-30 04:44:17]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-01 08:53:50]

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
File net/proxy/proxy_resolver_factory_mojo_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:273:
ASSERT_TRUE(client.encountered_error());
On 2016/06/29 22:53:49, leonhsl wrote:
> On 2016/06/29 21:36:20, yzshen1 wrote:
> > On 2016/06/27 11:03:16, leonhsl wrote:
> > > I think InterfacePtr would detect the connection error immediately once
> > > interface impl binding got closed, but here the thread always got hung
when
> I
> > > ran the test case. 
> > > Seems the connection error handler does not get called. Am I doing
something
> > > wrong here?
> > 
> > Please look at base::MessageLoop::ScopedNestableTaskAllower
> > The code has multiple layers of base::RunLoop, and the connection error
> handler
> > won't be run on this layer if nestable task is not allowed, so it will wait
> > forever.
> > 
> 
> Yeah I noticed the multiple layers of base::RunLoop but I did not go down
> deeply. Thanks a lot for the hints! Now I know where should I dip down,
firstly
> I'll try to understand base::MessageLoop::ScopedNestableTaskAllower there.

Done.

https://codereview.chromium.org/2096293002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo_unittest.cc:438: run_loop.RunUntilIdle();
On 2016/06/29 22:53:49, leonhsl wrote:
> On 2016/06/29 21:36:20, yzshen1 wrote:
> > Please note that RunUntilIdle is incorrect. The message loop is not
guaranteed
> > to be busy while we are waiting for the client to disconnect.
> 
> Understood.

Done.

[Elliot Glaysher, 2016-07-01 21:51:57]

I've gone back and forth on the filesystem case, but given that any
straightforward translation of current existing native file code would assume
blocking, I think I'm OK with [Sync] on these methods. (Which is why the ones
that are currently marked [Sync] are [Sync].)

components/filesystem/ lgtm.

[leonhsl(Using Gerrit), 2016-07-02 18:26:42]

Thanks Elliot a lot for confirmation of filesystem. And after investigated
around leveldb codes, I think we'd better not mark these interface functions as
[Sync], because leveldb production codes are using these async functions in many
places well, then it is supposed that test codes are using
WaitForIncomingResponse only for convenience. We should keep them async.

As for command_buffer and catalog, those affected functions are only called in
production code and are expected sync behavior, so I think we can mark them as
[Sync]. Also, from comments in run_loop.h, RunLoop is not allowed in production
code, means we can't use it to simulate WaitForIncomingResponse for them.

Uploaded ps#4, would you PTAnL? Thanks.

[leonhsl(Using Gerrit), 2016-07-03 07:19:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-03 07:19:14]

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-07-03 08:04:02]

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

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

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

[leonhsl(Using Gerrit), 2016-07-03 11:03:44]

Description was changed from

==========
Eliminate usage of InterfacePtr::WaitForIncomingResponse.

We'll replace them with either [Sync] call or a nested RunLoop waiting
asynchronously.

BUG=622438
==========

to

==========
Eliminate usage of InterfacePtr::WaitForIncomingResponse.

This CL removes InterfacePtr::WaitForIncomingResponse definition
and replaces usage of them with either [Sync] call or
a nested MessageLoop waiting asynchronously.

BUG=622438
==========

[leonhsl(Using Gerrit), 2016-07-03 11:05:42]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-03 11:05:48]

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-07-03 12:19:30]

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

[commit-bot: I haz the power, 2016-07-03 12:19:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-07-03 12:56:52]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-03 12:57:10]

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-07-03 14:03:02]

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

[commit-bot: I haz the power, 2016-07-03 14:03:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-07-03 15:02:26]

Uploaded ps#7, all code changes for this CL have been done, affected test cases
have been confirmed PASS locally, would you PTAnL? Thanks.

Elliot: OWNER review for components/leveldb/, mash/catalog_viewer/ and
services/ui/

sky: OWNER review for mash/catalog_viewer/ and services/ui/

agl: OWNER review for net/

Ken, Yuzhu: OWNER review for mojo/, and overall changes

[leonhsl(Using Gerrit), 2016-07-04 01:33:19]

leon.han@intel.com changed reviewers:
+ agl@chromium.org

[leonhsl(Using Gerrit), 2016-07-04 05:00:59]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 05:01:13]

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-07-04 05:07:06]

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

[commit-bot: I haz the power, 2016-07-04 05:07:07]

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

[leonhsl(Using Gerrit), 2016-07-05 02:15:25]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 02:15:33]

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-07-05 02:59:22]

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

[commit-bot: I haz the power, 2016-07-05 02:59:23]

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

[leonhsl(Using Gerrit), 2016-07-05 05:09:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 05:09:18]

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-07-05 05:58:07]

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

[commit-bot: I haz the power, 2016-07-05 05:58:08]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-06 15:47:37]

sky@chromium.org changed reviewers:
+ penghuang@chromium.org

[sky, 2016-07-06 15:47:38]

I'm not a good owner for command_buffer. +peng for that

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
File mash/catalog_viewer/catalog_viewer.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:58: OnGotCatalogEntries(entries);
There is no point in OnGotCatalogEntries now. Move the implementation here.

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:161:
base::WeakPtrFactory<CatalogViewerContents> weak_ptr_factory_;
I don't think you'll need this anymore.

[Elliot Glaysher, 2016-07-06 16:44:31]

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
File components/leveldb/leveldb_service_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:29: if (!quit_closure.is_null())
Is this ever actually null?

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:42: Capture(T1* t1, T2* t2, const
base::Closure& quit_closure = base::Closure()) {
Don't use default parameters.

[yzshen1, 2016-07-06 16:50:26]

Thanks!

I am not quite sure whether it is necessary to mark all methods in
components/filesystem as "sync". It seems a lot of those methods are only called
by testing code currently. But I defer to Elliot on this.

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
File components/leveldb/leveldb_service_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:46: void
DatabaseSyncPut(mojom::LevelDBDatabasePtr& database,
nit: |database| could be "mojom::LevelDBDatabase*" and the callsite will be
DatabaseSyncPut(database_ptr.get(), ...)

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
File mash/catalog_viewer/catalog_viewer.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:58: OnGotCatalogEntries(entries);
Not sure whether it matters, but please note that this is different from the
original code in that if the sync call fails (because of pipe disconnection),
OnGotCatalogEntries() stills runs. In this case, GetEntries() call will return
|false| and |entries| is untouched.

[Peng, 2016-07-06 17:31:02]

command buffer lgtm with some nits,

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/lib/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/command_buffer_client_impl.cc:80: bool handled =
command_buffer_->Initialize(
result = command_buffer_->Initialize(?

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/command_buffer_client_impl.cc:288: bool handled =
command_buffer_->MakeProgress(last_state_.get_offset, &state);
bool result = command_buffer_->MakeProgress(...?

[leonhsl(Using Gerrit), 2016-07-07 05:33:58]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 05:34:10]

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-07-07 05:55:17]

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

[commit-bot: I haz the power, 2016-07-07 05:55:18]

Dry run: 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_...)

[leonhsl(Using Gerrit), 2016-07-07 06:08:05]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 06:08:15]

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-07-07 07:14:20]

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

[commit-bot: I haz the power, 2016-07-07 07:14:22]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-07 07:43:12]

Thanks all a lot for review. Uploaded ps#8 and ps#9 to address comments, PTAnL,
Thanks.

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
File components/leveldb/leveldb_service_unittest.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:29: if (!quit_closure.is_null())
On 2016/07/06 16:44:30, Elliot Glaysher wrote:
> Is this ever actually null?

Removed the null check. I was assuming that some callers of Capture() maybe do
not need |quit_closure|, but it comes out that actually such case does not
exist.

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:42: Capture(T1* t1, T2* t2, const
base::Closure& quit_closure = base::Closure()) {
On 2016/07/06 16:44:30, Elliot Glaysher wrote:
> Don't use default parameters.

Done.

https://codereview.chromium.org/2096293002/diff/120001/components/leveldb/lev...
components/leveldb/leveldb_service_unittest.cc:46: void
DatabaseSyncPut(mojom::LevelDBDatabasePtr& database,
On 2016/07/06 16:50:26, yzshen1 wrote:
> nit: |database| could be "mojom::LevelDBDatabase*" and the callsite will be
> DatabaseSyncPut(database_ptr.get(), ...)

Done.

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
File mash/catalog_viewer/catalog_viewer.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:58: OnGotCatalogEntries(entries);
On 2016/07/06 16:50:26, yzshen1 wrote:
> Not sure whether it matters, but please note that this is different from the
> original code in that if the sync call fails (because of pipe disconnection),
> OnGotCatalogEntries() stills runs. In this case, GetEntries() call will return
> |false| and |entries| is untouched.

Yeah this indeed becomes different with before.. Thanks! I think we'd better
align with the original logic. Done.

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:58: OnGotCatalogEntries(entries);
On 2016/07/06 15:47:38, sky wrote:
> There is no point in OnGotCatalogEntries now. Move the implementation here. 

Moved OnGotCatalogEntries() impl here.

https://codereview.chromium.org/2096293002/diff/120001/mash/catalog_viewer/ca...
mash/catalog_viewer/catalog_viewer.cc:161:
base::WeakPtrFactory<CatalogViewerContents> weak_ptr_factory_;
On 2016/07/06 15:47:38, sky wrote:
> I don't think you'll need this anymore.

Yeah~ Done and Thanks!

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/lib/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/command_buffer_client_impl.cc:80: bool handled =
command_buffer_->Initialize(
On 2016/07/06 17:31:02, Peng wrote:
> result = command_buffer_->Initialize(?

Done.

https://codereview.chromium.org/2096293002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/command_buffer_client_impl.cc:288: bool handled =
command_buffer_->MakeProgress(last_state_.get_offset, &state);
On 2016/07/06 17:31:02, Peng wrote:
> bool result = command_buffer_->MakeProgress(...?

Done.

[yzshen1, 2016-07-07 15:42:33]

LGTM

[Elliot Glaysher, 2016-07-07 17:18:29]

lgtm

[agl, 2016-07-07 17:34:03]

Rubber-stamp LGTM for net/

[leonhsl(Using Gerrit), 2016-07-08 05:06:12]

leon.han@intel.com changed reviewers:
+ dcheng@chromium.org

[leonhsl(Using Gerrit), 2016-07-08 05:06:13]

Hi, Daniel, would you PTAL for OWNER review of mojom changes: marked several
functions as [Sync]. Thanks~

[dcheng, 2016-07-08 06:25:37]

rs lgtm for mojom changes

[leonhsl(Using Gerrit), 2016-07-09 08:24:49]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-07-09 08:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
penghuang@chromium.org
Link to the patchset: https://codereview.chromium.org/2096293002/#ps160001
(title: "Fix trybots failure")

[commit-bot: I haz the power, 2016-07-09 08:25: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-07-09 10:25:38]

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

[commit-bot: I haz the power, 2016-07-09 10:25:41]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[leonhsl(Using Gerrit), 2016-07-09 10:28:37]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2016-07-09 10:28:49]

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-07-09 12:29:42]

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

[commit-bot: I haz the power, 2016-07-09 12:29:44]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[leonhsl(Using Gerrit), 2016-07-10 05:24:13]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2016-07-10 05:24:23]

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-07-10 05:32:07]

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

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

Failed to apply patch for components/leveldb/leveldb_service_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed: components/leveldb/leveldb_service_unittest.cc:4
  Falling back to three-way merge...
  Applied patch to 'components/leveldb/leveldb_service_unittest.cc' with
conflicts.
  U components/leveldb/leveldb_service_unittest.cc

Patch:       components/leveldb/leveldb_service_unittest.cc
Index: components/leveldb/leveldb_service_unittest.cc
diff --git a/components/leveldb/leveldb_service_unittest.cc
b/components/leveldb/leveldb_service_unittest.cc
index
893ea6f6d160db76eb4427beb25fc53654201b79..88b1050a54d2594ceb2d673c702efecf359e2ff0
100644
--- a/components/leveldb/leveldb_service_unittest.cc
+++ b/components/leveldb/leveldb_service_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "components/filesystem/public/interfaces/directory.mojom.h"
 #include "components/filesystem/public/interfaces/file_system.mojom.h"
 #include "components/filesystem/public/interfaces/types.mojom.h"
@@ -21,18 +22,72 @@ namespace {
 template <typename... Args> void IgnoreAllArgs(Args&&...) {}
 
 template <typename... Args>
-void DoCaptures(Args*... out_args, Args... in_args) {
+void DoCaptures(Args*... out_args,
+                const base::Closure& quit_closure,
+                Args... in_args) {
   IgnoreAllArgs((*out_args = std::move(in_args))...);
+  quit_closure.Run();
 }
 
 template <typename T1>
-base::Callback<void(T1)> Capture(T1* t1) {
-  return base::Bind(&DoCaptures<T1>, t1);
+base::Callback<void(T1)> Capture(T1* t1, const base::Closure& quit_closure) {
+  return base::Bind(&DoCaptures<T1>, t1, quit_closure);
 }
 
 template <typename T1, typename T2>
-base::Callback<void(T1, T2)> Capture(T1* t1, T2* t2) {
-  return base::Bind(&DoCaptures<T1, T2>, t1, t2);
+base::Callback<void(T1, T2)> Capture(T1* t1,
+                                     T2* t2,
+                                     const base::Closure& quit_closure) {
+  return base::Bind(&DoCaptures<T1, T2>, t1, t2, quit_closure);
+}
+
+void DatabaseSyncPut(mojom::LevelDBDatabase* database,
+                     mojo::Array<uint8_t> key,
+                     mojo::Array<uint8_t> value,
+                     mojom::DatabaseError* out_error) {
+  base::RunLoop run_loop;
+  database->Put(std::move(key), std::move(value),
+                Capture(out_error, run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
+void DatabaseSyncGet(mojom::LevelDBDatabase* database,
+                     mojo::Array<uint8_t> key,
+                     mojom::DatabaseError* out_error,
+                     mojo::Array<uint8_t>* out_value) {
+  base::RunLoop run_loop;
+  database->Get(std::move(key),
+                Capture(out_error, out_value, run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
+void DatabaseSyncGetPrefixed(mojom::LevelDBDatabase* database,
+                             mojo::Array<uint8_t> key_prefix,
+                             mojom::DatabaseError* out_error,
+                             mojo::Array<mojom::KeyValuePtr>* out_key_values) {
+  base::RunLoop run_loop;
+  database->GetPrefixed(
+      std::move(key_prefix),
+      Capture(out_error, out_key_values, run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
+void DatabaseSyncDeletePrefixed(mojom::LevelDBDatabase* database,
+                                mojo::Array<uint8_t> key_prefix,
+                                mojom::DatabaseError* out_error) {
+  base::RunLoop run_loop;
+  database->DeletePrefixed(std::move(key_prefix),
+                           Capture(out_error, run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
+void LevelDBSyncOpenInMemory(mojom::LevelDBService* leveldb,
+                             mojom::LevelDBDatabaseRequest database,
+                             mojom::DatabaseError* out_error) {
+  base::RunLoop run_loop;
+  leveldb->OpenInMemory(std::move(database),
+                        Capture(out_error, run_loop.QuitClosure()));
+  run_loop.Run();
 }
 
 class LevelDBServiceTest : public shell::test::ShellTest {
@@ -58,8 +113,8 @@ class LevelDBServiceTest : public shell::test::ShellTest {
   // since |ASSERT_...()| doesn't work with return values.
   void GetTempDirectory(filesystem::mojom::DirectoryPtr* directory) {
     FileError error = FileError::FAILED;
-    files()->OpenTempDirectory(GetProxy(directory), Capture(&error));
-    ASSERT_TRUE(files().WaitForIncomingResponse());
+    bool handled = files()->OpenTempDirectory(GetProxy(directory), &error);
+    ASSERT_TRUE(handled);
     ASSERT_EQ(FileError::OK, error);
   }
 
@@ -76,40 +131,39 @@ class LevelDBServiceTest : public shell::test::ShellTest {
 TEST_F(LevelDBServiceTest, Basic) {
   mojom::DatabaseError error;
   mojom::LevelDBDatabasePtr database;
-  leveldb()->OpenInMemory(GetProxy(&database), Capture(&error));
-  ASSERT_TRUE(leveldb().WaitForIncomingResponse());
+  LevelDBSyncOpenInMemory(leveldb().get(), GetProxy(&database), &error);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Write a key to the database.
   error = mojom::DatabaseError::INVALID_ARGUMENT;
-  database->Put(mojo::Array<uint8_t>::From(std::string("key")),
-                mojo::Array<uint8_t>::From(std::string("value")),
-                Capture(&error));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncPut(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("key")),
+                  mojo::Array<uint8_t>::From(std::string("value")), &error);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Read the key back from the database.
   error = mojom::DatabaseError::INVALID_ARGUMENT;
   mojo::Array<uint8_t> value;
-  database->Get(mojo::Array<uint8_t>::From(std::string("key")),
-                Capture(&error, &value));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncGet(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("key")), &error,
+                  &value);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
   EXPECT_EQ("value", value.To<std::string>());
 
   // Delete the key from the database.
   error = mojom::DatabaseError::INVALID_ARGUMENT;
+  base::RunLoop run_loop;
   database->Delete(mojo::Array<uint8_t>::From(std::string("key")),
-                   Capture(&error));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+                   Capture(&error, run_loop.QuitClosure()));
+  run_loop.Run();
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Read the key back from the database.
   error = mojom::DatabaseError::INVALID_ARGUMENT;
   value.SetToEmpty();
-  database->Get(mojo::Array<uint8_t>::From(std::string("key")),
-                Capture(&error, &value));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncGet(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("key")), &error,
+                  &value);
   EXPECT_EQ(mojom::DatabaseError::NOT_FOUND, error);
   EXPECT_EQ("", value.To<std::string>());
 }
@@ -117,15 +171,13 @@ TEST_F(LevelDBServiceTest, Basic) {
 TEST_F(LevelDBServiceTest, WriteBatch) {
   mojom::DatabaseError error;
   mojom::LevelDBDatabasePtr database;
-  leveldb()->OpenInMemory(GetProxy(&database), Capture(&error));
-  ASSERT_TRUE(leveldb().WaitForIncomingResponse());
+  LevelDBSyncOpenInMemory(leveldb().get(), GetProxy(&database), &error);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Write a key to the database.
-  database->Put(mojo::Array<uint8_t>::From(std::string("key")),
-                mojo::Array<uint8_t>::From(std::string("value")),
-                Capture(&error));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncPut(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("key")),
+                  mojo::Array<uint8_t>::From(std::string("value")), &error);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Create a batched operation which both deletes "key" and adds another
write.
@@ -141,37 +193,37 @@ TEST_F(LevelDBServiceTest, WriteBatch) {
   item->value = mojo::Array<uint8_t>::From(std::string("more"));
   operations.push_back(std::move(item));
 
-  database->Write(std::move(operations), Capture(&error));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  base::RunLoop run_loop;
+  database->Write(std::move(operations),
+                  Capture(&error, run_loop.QuitClosure()));
+  run_loop.Run();
   EXPECT_EQ(mojom::DatabaseError::OK, error);
 
   // Reading "key" should be invalid now.
   error = mojom::DatabaseError::INVALID_ARGUMENT;
   mojo::Array<uint8_t> value;
-  database->Get(mojo::Array<uint8_t>::From(std::string("key")),
-                Capture(&error, &value));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncGet(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("key")), &error,
+                  &value);
   EXPECT_EQ(mojom::DatabaseError::NOT_FOUND, error);
   EXPECT_EQ("", value.To<std::string>());
 
   // Reading "other" should return "more"
   error = mojom::DatabaseError::INVALID_ARGUMENT;
-  database->Get(mojo::Array<uint8_t>::From(std::string("other")),
-                Capture(&error, &value));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncGet(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("other")), &error,
+                  &value);
   EXPECT_EQ(mojom::DatabaseError::OK, error);
   EXPECT_EQ("more", value.To<std::string>());
 
   // Write a some prefixed keys to the database.
-  database->Put(mojo::Array<uint8_t>::From(std::string("prefix-key1")),
-                mojo::Array<uint8_t>::From(std::string("value")),
-                Capture(&error));
-  ASSERT_TRUE(database.WaitForIncomingResponse());
+  DatabaseSyncPut(database.get(),
+                  mojo::Array<uint8_t>::From(std::string("prefix-key1")),
+                  mojo::Array<uint8_t>::From(std::string("value")), &error);
   EXPECT_…
(message too large)

[commit-bot: I haz the power, 2016-07-10 05:34:02]

Description was changed from

==========
Eliminate usage of InterfacePtr::WaitForIncomingResponse.

This CL removes InterfacePtr::WaitForIncomingResponse definition
and replaces usage of them with either [Sync] call or
a nested MessageLoop waiting asynchronously.

BUG=622438
==========

to

==========
Eliminate usage of InterfacePtr::WaitForIncomingResponse.

This CL removes InterfacePtr::WaitForIncomingResponse definition
and replaces usage of them with either [Sync] call or
a nested MessageLoop waiting asynchronously.

BUG=622438

Committed: https://crrev.com/83055f715f72c2394efd11b6bb17656899f4c9d0
Cr-Commit-Position: refs/heads/master@{#404579}
==========

[commit-bot: I haz the power, 2016-07-10 05:34:05]

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