[remoting android] Close navigation drawer before opening Help/Feedback.

[remoting android] Close navigation drawer before opening Help/Feedback.

This causes the Feedback screenshot to be captured after the drawer is
closed.

BUG=651527
Committed: https://crrev.com/c7c97a5c8c6b7f47b8205a7b21b27590e0a08d35
Cr-Commit-Position: refs/heads/master@{#423024}

[Lambros, 2016-10-04 23:01:34]

lambroslambrou@chromium.org changed reviewers:
+ yuweih@chromium.org

[Lambros, 2016-10-04 23:01:51]

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

[commit-bot: I haz the power, 2016-10-04 23:02:35]

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-10-04 23:41:08]

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

[commit-bot: I haz the power, 2016-10-04 23:41:08]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2016-10-04 23:41:15]

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:178: Runnable
task = mPendingDrawerCloseTask;
Optionally you can have a queue of pending tasks rather than just one task so
that closeDrawerThenRun() can be run multiple times in a task.

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:185: private
void closeDrawerThenRun(Runnable task) {
Instead of having extra launch* methods here, could it be better to make
closeDrawerThenRun() public then create the anonymous class in
NavigationMenuAdapter? By doing so we don't need to go back and forth between
Chromoting and the Adapter when we later change the list items.

[Lambros, 2016-10-05 00:03:15]

Thanks for taking a look.

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:178: Runnable
task = mPendingDrawerCloseTask;
On 2016/10/04 23:41:14, Yuwei wrote:
> Optionally you can have a queue of pending tasks rather than just one task so
> that closeDrawerThenRun() can be run multiple times in a task.

Acknowledged. No need for a queue just yet.

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:185: private
void closeDrawerThenRun(Runnable task) {
On 2016/10/04 23:41:14, Yuwei wrote:
> Instead of having extra launch* methods here, could it be better to make
> closeDrawerThenRun() public then create the anonymous class in
> NavigationMenuAdapter? By doing so we don't need to go back and forth between
> Chromoting and the Adapter when we later change the list items.

If I'm not mistaken, this would mean doubly-nested anonymous Runnable classes in
NavigationMenuAdapter which would look really ugly.
That could be avoided by pulling out the Runnable-creation into new methods, but
that doesn't seem any better or worse than what I've got now.

[Yuwei, 2016-10-05 00:27:02]

LGTM

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/2389153003/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:185: private
void closeDrawerThenRun(Runnable task) {
On 2016/10/05 00:03:15, Lambros wrote:
> On 2016/10/04 23:41:14, Yuwei wrote:
> > Instead of having extra launch* methods here, could it be better to make
> > closeDrawerThenRun() public then create the anonymous class in
> > NavigationMenuAdapter? By doing so we don't need to go back and forth
between
> > Chromoting and the Adapter when we later change the list items.
> 
> If I'm not mistaken, this would mean doubly-nested anonymous Runnable classes
in
> NavigationMenuAdapter which would look really ugly.

Yep. Callback hell :) Though it can be simplified to:

()->{closeDrawerThenRun(()->{HelpSingleton.get().launchFeedback();});}

once the N SDK get rolled in.

> That could be avoided by pulling out the Runnable-creation into new methods,

Or you can just create them at the beginning of createNavigationMenu().

Maybe we can later implement the Promise and have something like this:

item.onClick().then(()->{
  return chromoting.closeDrawer();
}).then(()->{
  launchHelp(...);
});

> but that doesn't seem any better or worse than what I've got now.

Since Chromoting.java is already too messy to read, I would try not to introduce
more unrelated logic to this class. I don't really have a strong opinion for
which one is better, so you can go ahead if you like.

[Lambros, 2016-10-05 00:35:00]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2016-10-05 00:35:25]

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-10-05 00:42:11]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-05 00:50:22]

Description was changed from

==========
[remoting android] Close navigation drawer before opening Help/Feedback.

This causes the Feedback screenshot to be captured after the drawer is
closed.

BUG=651527
==========

to

==========
[remoting android] Close navigation drawer before opening Help/Feedback.

This causes the Feedback screenshot to be captured after the drawer is
closed.

BUG=651527

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

[commit-bot: I haz the power, 2016-10-05 00:50:23]

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