[Home] Add lifecycle events and some tests

[Home] Add lifecycle events and some tests

This change adds the following lifecycle events to the
BottomSheetObserver:

- onSheetOpened(): A notification that the sheet has an offset from
    the peeking state that is greater than 0.
- onSheetClosed(): A notification that the sheet has completely
    returned to the its peeking state.
- onLoadUrl(): A notification that a URL has started to load.

The change also includes tests for the new events.

BUG=689961
Review-Url: https://codereview.chromium.org/2698613006
Cr-Commit-Position: refs/heads/master@{#452107}
Committed: https://chromium.googlesource.com/chromium/src/+/f9f64be3422ea78949a630766ad580a87b50e516

[mdjones, 2017-02-17 16:20:15]

Description was changed from

==========
[Home] Add lifecycle events and some tests

This change adds the following lifecycle events to the
BottomSheetObserver:

- onSheetOpened(): A notification that the sheet has an offset from
    the peeking state that is greater than 0.
- onSheetClosed(): A notification that the sheet has completely
    returned to the its peeking state.
- onLoadUrl(): A notification that a URL has started to load.

The change also includes tests for the new events.

BUG=689961

[Home] Add events to BottomSheetObserver
==========

to

==========
[Home] Add lifecycle events and some tests

This change adds the following lifecycle events to the
BottomSheetObserver:

- onSheetOpened(): A notification that the sheet has an offset from
    the peeking state that is greater than 0.
- onSheetClosed(): A notification that the sheet has completely
    returned to the its peeking state.
- onLoadUrl(): A notification that a URL has started to load.

The change also includes tests for the new events.

BUG=689961
==========

[mdjones, 2017-02-17 16:20:45]

mdjones@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dgn@chromium.org

[mdjones, 2017-02-17 16:20:46]

The bulk of this change is tests. ptal

[mdjones, 2017-02-17 16:47:16]

mdjones@chromium.org changed reviewers:
+ twellington@chromium.org

[mdjones, 2017-02-17 16:47:16]

+twellington since she is starting to work with this.

[Theresa, 2017-02-17 18:54:48]

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:396:
for (BottomSheetObserver o : mObservers) o.onLoadUrl(params.getUrl());
Should this happen after the early return for native page URLs?

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:618:
&& getSheetOffsetFromBottom() > getMinOffset()) {
Would this second line fail if the sheet were offscreen (e.g. due to a scroll)?

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:52:
mOpenedCount++;
mOpenedCallbackHelper and mClosedCallbackHelper keep track of the number of
times they've been called so this is a bit redundant.

[gone, 2017-02-17 20:02:33]

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:611:
public void setSheetOffsetFromBottom(float offset) {
If you feel strongly enough to yell, maybe keep this method private and add a
method like setSheetOffsetFromBottomForTests()?

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:36:
private CallbackHelper mClosedCallbackHelper = new CallbackHelper();
finals, move upwards

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:165:
// Make sure the close event was only called once.
Don't know if the comments here buy you much.  You can infer that from the + 1
value

[mdjones, 2017-02-17 21:33:19]

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:396:
for (BottomSheetObserver o : mObservers) o.onLoadUrl(params.getUrl());
On 2017/02/17 18:54:48, Theresa wrote:
> Should this happen after the early return for native page URLs?

That depends on how we consider navigations inside of native pages. The reason
the early return is there is so we don't use the tab's native handling logic. My
thinking was that it is still *technically* loading a URL so it would be here.
If you feel strongly the other way, I can move it.

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:611:
public void setSheetOffsetFromBottom(float offset) {
On 2017/02/17 20:02:32, dfalcantara (load balance plz) wrote:
> If you feel strongly enough to yell, maybe keep this method private and add a
> method like setSheetOffsetFromBottomForTests()?

Good plan; done.

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:618:
&& getSheetOffsetFromBottom() > getMinOffset()) {
On 2017/02/17 18:54:48, Theresa wrote:
> Would this second line fail if the sheet were offscreen (e.g. due to a
scroll)?

Yes but the sheet's android view never moves below the peeking state, it is just
invisible. If that line fails, it also indicates that the sheet is moving up
instead of down, so we wouldn't want the event to trigger anyway.

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:36:
private CallbackHelper mClosedCallbackHelper = new CallbackHelper();
On 2017/02/17 20:02:32, dfalcantara (load balance plz) wrote:
> finals, move upwards

Done.

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:52:
mOpenedCount++;
On 2017/02/17 18:54:48, Theresa wrote:
> mOpenedCallbackHelper and mClosedCallbackHelper keep track of the number of
> times they've been called so this is a bit redundant.

Removed in favor of the one CallbackHelper has.

https://codereview.chromium.org/2698613006/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:165:
// Make sure the close event was only called once.
On 2017/02/17 20:02:32, dfalcantara (load balance plz) wrote:
> Don't know if the comments here buy you much.  You can infer that from the + 1
> value

Removed.

[Theresa, 2017-02-17 21:48:08]

lgtm

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:396:
for (BottomSheetObserver o : mObservers) o.onLoadUrl(params.getUrl());
On 2017/02/17 21:33:19, mdjones wrote:
> On 2017/02/17 18:54:48, Theresa wrote:
> > Should this happen after the early return for native page URLs?
> 
> That depends on how we consider navigations inside of native pages. The reason
> the early return is there is so we don't use the tab's native handling logic.

The early return is to handle bookmarks/downloads changing the URL?

> My
> thinking was that it is still *technically* loading a URL so it would be here.
> If you feel strongly the other way, I can move it.

I don't feel strongly about it.

[Theresa, 2017-02-17 21:48:09]

lgtm

[mdjones, 2017-02-17 21:52:11]

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:396:
for (BottomSheetObserver o : mObservers) o.onLoadUrl(params.getUrl());
On 2017/02/17 21:48:07, Theresa wrote:
> On 2017/02/17 21:33:19, mdjones wrote:
> > On 2017/02/17 18:54:48, Theresa wrote:
> > > Should this happen after the early return for native page URLs?
> > 
> > That depends on how we consider navigations inside of native pages. The
reason
> > the early return is there is so we don't use the tab's native handling
logic.
> 
> The early return is to handle bookmarks/downloads changing the URL?
That's the plan right now. There needs to be some more plumbing before that
works though. Things will be more concrete when we start adding the other chrome
home content.

> 
> > My
> > thinking was that it is still *technically* loading a URL so it would be
here.
> > If you feel strongly the other way, I can move it.
> 
> I don't feel strongly about it.

[gone, 2017-02-17 21:59:19]

lgtm

[Theresa, 2017-02-17 23:15:36]

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

[commit-bot: I haz the power, 2017-02-17 23:16:44]

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, 2017-02-18 01:19:45]

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

[commit-bot: I haz the power, 2017-02-18 01:19:46]

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

[dgn, 2017-02-20 11:37:14]

Thanks! lgtm

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:614:
if (MathUtils.areFloatsEqual(offset, getMinOffset())
nit: else if to make it more easily spottable that onSheetOpened() and
onSheetClosed are mutually exclusive?

That being said, I'm not sure they currently are, since areFloatsEqual()  uses
an epsilon and the > operator doesn't take it into account.

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java
(right):

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:88:
private void setSheetState(final int state, final boolean animate) {
maybe move these utility methods to the base test class?

[mdjones, 2017-02-21 17:19:11]

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:614:
if (MathUtils.areFloatsEqual(offset, getMinOffset())
On 2017/02/20 11:37:14, dgn wrote:
> nit: else if to make it more easily spottable that onSheetOpened() and
> onSheetClosed are mutually exclusive?
> 
> That being said, I'm not sure they currently are, since areFloatsEqual()  uses
> an epsilon and the > operator doesn't take it into account.

Done. One option to make this more clear is setting the exact height at the
target state after the animation ends. The problem is that float == float is
still going to make presubmit angry and we would have to add code to make sure
the event runs a single time. I think we are going to hit the overlap case
rarely if ever and if we do, we can take another look at this.

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java
(right):

https://codereview.chromium.org/2698613006/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/BottomSheetObserverTest.java:88:
private void setSheetState(final int state, final boolean animate) {
On 2017/02/20 11:37:14, dgn wrote:
> maybe move these utility methods to the base test class?

Done.

[mdjones, 2017-02-21 17:21:10]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-02-21 17:21:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dfalcantara@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2698613006/#ps60001
(title: "protected test methods")

[commit-bot: I haz the power, 2017-02-21 17:21:51]

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, 2017-02-21 17:48:20]

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

[commit-bot: I haz the power, 2017-02-21 17:48:23]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mdjones, 2017-02-21 19:10:05]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-02-21 19:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dfalcantara@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2698613006/#ps80001
(title: "move bottom sheet handle")

[commit-bot: I haz the power, 2017-02-21 19:10:52]

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, 2017-02-21 21:14:27]

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

[commit-bot: I haz the power, 2017-02-21 21:14:28]

Try jobs failed on following builders:
  chromium_presubmit 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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[mdjones, 2017-02-21 23:22:40]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-02-21 23:22:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dfalcantara@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2698613006/#ps100001
(title: "fix previous patch")

[commit-bot: I haz the power, 2017-02-21 23:24:16]

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, 2017-02-22 01:27:15]

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

[commit-bot: I haz the power, 2017-02-22 01:27:16]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_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_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)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[mdjones, 2017-02-22 16:28:11]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2017-02-22 16:28:44]

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, 2017-02-22 17:23:35]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487780891675240,
"parent_rev": "a78746ec1a1bfa668f5bcb01d2b2665d2c514369", "commit_rev":
"f9f64be3422ea78949a630766ad580a87b50e516"}

[commit-bot: I haz the power, 2017-02-22 17:24:17]

Description was changed from

==========
[Home] Add lifecycle events and some tests

This change adds the following lifecycle events to the
BottomSheetObserver:

- onSheetOpened(): A notification that the sheet has an offset from
    the peeking state that is greater than 0.
- onSheetClosed(): A notification that the sheet has completely
    returned to the its peeking state.
- onLoadUrl(): A notification that a URL has started to load.

The change also includes tests for the new events.

BUG=689961
==========

to

==========
[Home] Add lifecycle events and some tests

This change adds the following lifecycle events to the
BottomSheetObserver:

- onSheetOpened(): A notification that the sheet has an offset from
    the peeking state that is greater than 0.
- onSheetClosed(): A notification that the sheet has completely
    returned to the its peeking state.
- onLoadUrl(): A notification that a URL has started to load.

The change also includes tests for the new events.

BUG=689961

Review-Url: https://codereview.chromium.org/2698613006
Cr-Commit-Position: refs/heads/master@{#452107}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9f64be3422ea78949a630766ad5...
==========

[commit-bot: I haz the power, 2017-02-22 17:24:20]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f9f64be3422ea78949a630766ad5...