1500644 - Make geckoview_example's toolbar behave like Focus's dynamic toolbar

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

This was a feature request from the October 2018 performance work week. I don't exactly what about geckoview_example's toolbar needs to change.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Chris will get more details.

Comment 2

[Chris Peterson [:cpeterson]]

Markus, did you ask for this toolbar fix at the Toronto performance work week? What was the specific change? For geckoview_example to resize the viewport to account for the toolbar size?

Comment 3

[Markus Stange [:mstange]]

Yes, that was me.

Focus moves the GeckoView during scrolling when it hides and unhides its toolbar. Initially I thought it was also resizing the view, but it seems that's not the case; things at the bottom just get cut off while the toolbar is showing.
Moving and resizing a view can have an impact on performance. Since our performance investigations and profiling are going to happen in the GeckoView example, I'd like GeckoView example to work as similarly to Focus as possible in all ways that can affect performance.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

It would be really great that GekckoView example has the dynamic toolbar feature. (and GeckoViewTestActivity). The dynamic toolbar in android-component is implemented with using CoordinatorLayout which seems to have introduced in API 24.1, but GeckoView example and GeckoViewTestActivity are for testing purpose so that we can use CoordinatorLayout?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1500644 - Make GeckoView example toolbar dynamic at bottom by using CoordinatorLayout. r?#geckoview-reviewers

This way is basically how android-components provides the dynamic toolbar
feature but there are a couple of caveats on this change as of now.

1. The toolbar doesn't snap back/forward at all when the user leaves their
   finger in the middle of the toolbar's transition
2. There is no option to switch the toolbar position at top
3. This change forces the toolbar to be dynamic, there is no way to make
   it static

Each corresponding file in android-components is;
NestedGeckoView.java <- NestedGeckoView.kt [1]
ToolbarBottomBehavior.java <- BrowserToolbarBottomBehavior.kt [2]
GeckoViewBottomBehavior <- EngineViewBottomBehavior.kt [3]

[1] https://github.com/mozilla-mobile/android-components/blob/b6a5c6444140920768e1332e2618538efe29151e/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt
[2] https://github.com/mozilla-mobile/android-components/blob/b6a5c6444140920768e1332e2618538efe29151e/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBottomBehavior.kt
[3] https://github.com/mozilla-mobile/android-components/blob/b6a5c6444140920768e1332e2618538efe29151e/components/feature/session/src/main/java/mozilla/components/feature/session/behavior/EngineViewBottomBehavior.kt

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I just pushed the current state of the change for this bug, but there is one thing I don't quite understand. That is that an equivalent way in Java as coroutine in Kotlin.

In android-components a coroutine is used to handle the result of onTouchEventForResult, without the coroutine equivalent thing, it looks as it works fine, but there may be race conditions it won't work. So I wonder how we can do the same manner in Java. (I also don't quite understand what value should be returned from the onTouchEventForResult's then lambda.

:snorp, or Agi, can you please shed some lights on that?

(CCing Petru)

Comment 7

[Petru-Mugurel Lingurar [:petru]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

I just pushed the current state of the change for this bug, but there is one thing I don't quite understand. That is that an equivalent way in Java as coroutine in Kotlin.

In android-components a coroutine is used to handle the result of onTouchEventForResult, without the coroutine equivalent thing, it looks as it works fine, but there may be race conditions it won't work. So I wonder how we can do the same manner in Java. (I also don't quite understand what value should be returned from the onTouchEventForResult's then lambda.

(CCing Petru)

The launch coroutine builder can be seen as starting an operation to be done in background, without waiting for a response. Fire and forget.

It is used here when the user first touches the screen to decouple GV#onTouchEventForResult off of Main, as waiting for a response from that could take upwards of 500ms (seen on google maps on a Pixel 3, possibly more on older devices) to return how that touch was handled (INPUT_RESULT_UNHANDLED / INPUT_RESULT_HANDLED / INPUT_RESULT_HANDLED_CONTENT).
.then I understand registers a listener for when the GeckoResult resolves so I'd think we're good, no other changes needed.
Don't know about the return of ".then".

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

In android-components a coroutine is used to handle the result of onTouchEventForResult, without the coroutine equivalent thing, it looks as it works fine, but there may be race conditions it won't work. So I wonder how we can do the same manner in Java. (I also don't quite understand what value should be returned from the onTouchEventForResult's then lambda.

:snorp, or Agi, can you please shed some lights on that?

The return result of then is used if you want to chain GeckoResults, e.g. like this

result.then(result -> {
      // code
      return doExpensiveAction();
}).then(expensiveActionResult -> {

}).then(...);

If you just want to consume the result you can use accept. E.g. in your code:

            super.onTouchEventForResult(event).accept(inputResult -> {
                mInputResult = inputResult;
                startNestedScroll(ViewCompat.SCROLL_AXIS_VERTICAL);
            });

I'm don't really know anything about coroutines but if it's something like async javascript methods then your java code should be pretty much equivalent to the kotlin code that you linked.

The code running in accept will run in the calling thread, so if this is always called from the same thread there shouldn't be any race condition going on (and I think that this is the case here, since onTouchEvent should be called from the UI thread).

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Thank you Agi, that's all what I wanted to know. (and thank you Petru)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

This way is basically how android-components provides the dynamic toolbar
feature but there are a couple of caveats on this change as of now.

1. The toolbar doesn't snap back/forward at all when the user leaves their
   finger in the middle of the toolbar's transition

I did implement the snap feature in the same way what A-C does.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ecb44816920
Make GeckoView example toolbar dynamic at bottom by using CoordinatorLayout. r=geckoview-reviewers,snorp

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4ecb44816920