🏠 Add instrumentation test for Suggestions BottomSheet

[Home] Add instrumentation test for Suggestions BottomSheet

Add a BottomSheetTestCaseBase to set up ChromeHome compatible test
cases. Add a test for issue 688391 to verify the touch interception
behaviour associated to the context menu manager.

BUG=688391, 671361
Review-Url: https://codereview.chromium.org/2670863004
Cr-Commit-Position: refs/heads/master@{#450492}
Committed: https://chromium.googlesource.com/chromium/src/+/a43c4f909bf3a4ee95a5b2739fe1175ff9080fd5

[dgn, 2017-02-03 17:18:25]

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

[dgn, 2017-02-03 17:18:26]

PTAL

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

https://codereview.chromium.org/2670863004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:554:
if (mCurrentState != SHEET_STATE_PEEK) onExitPeekState();
added that to instantiate the bottomsheet when calling setSheetState()

[dgn, 2017-02-03 17:20:29]

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

[commit-bot: I haz the power, 2017-02-03 17:20:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2017-02-03 17:42:45]

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

[commit-bot: I haz the power, 2017-02-03 17:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mdjones, 2017-02-03 18:05:40]

Thanks for adding the missing annotations in BottomSheet!

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

https://codereview.chromium.org/2670863004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:556:
if (mCurrentState != SHEET_STATE_PEEK) onExitPeekState();
This will be called for cases like half->full and full->half.

For this event to work properly, we should change the settle animator to call
setSheetOffsetFromBottom instead of directly changing View.TRANSLATION_Y (using
AnimationUpdateListener). That way, we could just add the following check in
setSheetOffsetFromBottom:

if (getSheetOffsetFromBottom() == getMinOffset() && offset > getMinOffset()) {
    onExitPeekState();
}

In this case we can also remove lines 208-212.

I can whip up a patch to do this if you're ok with waiting a few hours for it to
land (I know I should have originally handled this case). If not, you can drop
it into this patch.

[dgn, 2017-02-03 18:08:53]

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

https://codereview.chromium.org/2670863004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:556:
if (mCurrentState != SHEET_STATE_PEEK) onExitPeekState();
On 2017/02/03 18:05:39, mdjones wrote:
> This will be called for cases like half->full and full->half.
> 
I figured that since we check that the page has changed before doing anything it
wasn't a big deal.

> For this event to work properly, we should change the settle animator to call
> setSheetOffsetFromBottom instead of directly changing View.TRANSLATION_Y
(using
> AnimationUpdateListener). That way, we could just add the following check in
> setSheetOffsetFromBottom:
> 
> if (getSheetOffsetFromBottom() == getMinOffset() && offset > getMinOffset()) {
>     onExitPeekState();
> }
> 
> In this case we can also remove lines 208-212.
> 
> I can whip up a patch to do this if you're ok with waiting a few hours for it
to
> land (I know I should have originally handled this case). If not, you can drop
> it into this patch.

Thanks I would be grateful if you could make this change. I'm not touching the
patch until Monday anyway :)

[commit-bot: I haz the power, 2017-02-03 18:45:51]

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

[commit-bot: I haz the power, 2017-02-03 18:45:52]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-02-03 20:02:19]

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

https://codereview.chromium.org/2670863004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:556:
if (mCurrentState != SHEET_STATE_PEEK) onExitPeekState();
On 2017/02/03 18:08:53, dgn wrote:
> On 2017/02/03 18:05:39, mdjones wrote:
> > This will be called for cases like half->full and full->half.
> > 
> I figured that since we check that the page has changed before doing anything
it
> wasn't a big deal.
> 
> > For this event to work properly, we should change the settle animator to
call
> > setSheetOffsetFromBottom instead of directly changing View.TRANSLATION_Y
> (using
> > AnimationUpdateListener). That way, we could just add the following check in
> > setSheetOffsetFromBottom:
> > 
> > if (getSheetOffsetFromBottom() == getMinOffset() && offset > getMinOffset())
{
> >     onExitPeekState();
> > }
> > 
> > In this case we can also remove lines 208-212.
> > 
> > I can whip up a patch to do this if you're ok with waiting a few hours for
it
> to
> > land (I know I should have originally handled this case). If not, you can
drop
> > it into this patch.
> 
> Thanks I would be grateful if you could make this change. I'm not touching the
> patch until Monday anyway :)

https://codereview.chromium.org/2676663004/

[Michael van Ouwerkerk, 2017-02-03 20:20:33]

Nice to see our first Zine in Chrome Home tests!

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
(right):

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:16:
* Tests for the behaviour of the suggestions bottom sheet content.
How about this? "Instrumentation tests for {@code
SuggestionsBottomSheetContent}."

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:25:
TestTouchUtils.longClickView(getInstrumentation(),
recyclerView.findFirstCard().itemView);
Do we know for sure that the first card has been rendered at this point? I ran
into lots of edge cases around that, see NewTabPageRecyclerViewTest#waitForView.

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:32:
private static MotionEvent createTap() {
Maybe "createTapEvent"?

https://codereview.chromium.org/2670863004/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
(right):

https://codereview.chromium.org/2670863004/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java:30:
getInstrumentation().waitForIdleSync();
Could we wait for something more specific? Is there some kind of state we can
observe in the bottom sheet, or a callback that we can wait for?

[dgn, 2017-02-06 15:50:17]

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

[commit-bot: I haz the power, 2017-02-06 15:50:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2017-02-06 16:45:18]

PTAL. I wish the line diff count was correct :/

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
(right):

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:16:
* Tests for the behaviour of the suggestions bottom sheet content.
On 2017/02/03 20:20:33, Michael van Ouwerkerk wrote:
> How about this? "Instrumentation tests for {@code
> SuggestionsBottomSheetContent}."

Done.

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:25:
TestTouchUtils.longClickView(getInstrumentation(),
recyclerView.findFirstCard().itemView);
On 2017/02/03 20:20:33, Michael van Ouwerkerk wrote:
> Do we know for sure that the first card has been rendered at this point? I ran
> into lots of edge cases around that, see
NewTabPageRecyclerViewTest#waitForView.

Done.

https://codereview.chromium.org/2670863004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:32:
private static MotionEvent createTap() {
On 2017/02/03 20:20:33, Michael van Ouwerkerk wrote:
> Maybe "createTapEvent"?

Done.

[dgn, 2017-02-06 16:45:27]

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

[commit-bot: I haz the power, 2017-02-06 16:46:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2017-02-06 16:51:23]

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:99:
private static SuggestionsUiDelegateImpl createSuggestionsDelegate(
not really happy about this bit... the main issue is that SnippetsBridge needs
to be destroyed, which is considered an implementation detail as far as the
SuggestionSource interface goes. So we can't have the fake one implement it
right now, and we end up having a special behaviour (destroying the bridge) that
is not exercised in the test. Will revisit this bit after I look into removing
the reliance on the manager to get dependencies.

[commit-bot: I haz the power, 2017-02-06 18:04:41]

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

[commit-bot: I haz the power, 2017-02-06 18:04:42]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-02-07 11:12:41]

lgtm with nits Thanks!

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:27:
implements SuggestionsSource, SuggestionsMetricsReporter, DestructionObserver {
Maybe SuggestionsSource could be a DestructionObserver? Also fine for a later
CL.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java_sou...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java_sou...
chrome/android/java_sources.gni:1543:
"junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java",
nit: Maybe just "ContentSuggestionsUnitTestUtils"?

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:43:
mSuggestionsSource = null;
nit: I think we need not null out any instance members.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java:16:
* {@link ContentSuggestionsTestUtils}
nit: close with a period.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java:18:
public class ContentSuggestionsJUnitTestUtils {
nit: Please make this static util class final and give it a private constructor.

https://codereview.chromium.org/2670863004/diff/80001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java:22:
.setChromeHomeEnabled(true);
Why do we need both this and "
@CommandLineFlags.Add({"enable-features=ChromeHome"})"?

[Finnur, 2017-02-09 10:43:45]

I'm very interested in using RecyclerViewTestUtils.java when it becomes
available. :)

[dgn, 2017-02-09 12:10:18]

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

[dgn, 2017-02-09 12:10:19]

dfalcantara@chromium.org: PTAL

mdjones: friendly ping :) What's the status for the onExitPeekingState changes?

[gone, 2017-02-10 18:17:09]

lgtm % michael's comments

You can just use this when multiple bugs are involved:
BUG=671361,688391

Monorail still updates all the bugs accordingly.

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

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:279:
@SheetState
I'd just put @SheetState on the same line to make it more obvious what it goes
to.

@SheetState int targetState =
        getTargetSheetState(etc)

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:405:
* @param content The BottomSheetContent to show.
{@link BottomSheetContent}

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:612:
int nextState = sStates[0];
Ditto about same line annotations.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:40:
super.tearDown();
I usually like putting super.tearDown() at the bottom to guarantee whatever your
cleanup code relies on is still there.  Doesn't seem like you need to do it
here, but it's good practice.

[dgn, 2017-02-13 14:25:39]

PTAL

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

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:279:
@SheetState
On 2017/02/10 18:17:08, dfalcantara (load balance plz) wrote:
> I'd just put @SheetState on the same line to make it more obvious what it goes
> to.
> 
> @SheetState int targetState =
>         getTargetSheetState(etc)

That's the style recommended by `git cl format`. It's also consistent with the
other annotations across the codebase. At least in the files I usually look at.
I don't really want to change that if it starts polluting other people's CLs
when the also use git cl format.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:405:
* @param content The BottomSheetContent to show.
On 2017/02/10 18:17:08, dfalcantara (load balance plz) wrote:
> {@link BottomSheetContent}

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java_sou...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java_sou...
chrome/android/java_sources.gni:1543:
"junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java",
On 2017/02/07 11:12:41, Michael van Ouwerkerk wrote:
> nit: Maybe just "ContentSuggestionsUnitTestUtils"?

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:40:
super.tearDown();
On 2017/02/10 18:17:09, dfalcantara (load balance plz) wrote:
> I usually like putting super.tearDown() at the bottom to guarantee whatever
your
> cleanup code relies on is still there.  Doesn't seem like you need to do it
> here, but it's good practice.

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java:43:
mSuggestionsSource = null;
On 2017/02/07 11:12:41, Michael van Ouwerkerk wrote:
> nit: I think we need not null out any instance members.

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java:16:
* {@link ContentSuggestionsTestUtils}
On 2017/02/07 11:12:41, Michael van Ouwerkerk wrote:
> nit: close with a period.

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsJUnitTestUtils.java:18:
public class ContentSuggestionsJUnitTestUtils {
On 2017/02/07 11:12:41, Michael van Ouwerkerk wrote:
> nit: Please make this static util class final and give it a private
constructor.

Done.

https://codereview.chromium.org/2670863004/diff/80001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
(right):

https://codereview.chromium.org/2670863004/diff/80001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java:22:
.setChromeHomeEnabled(true);
On 2017/02/07 11:12:41, Michael van Ouwerkerk wrote:
> Why do we need both this and "
> @CommandLineFlags.Add({"enable-features=ChromeHome"})"?

I looked more into why I needed this. There is an initialisation issue that
happens in instrumentation tests. Filed https://crbug.com/691563

[dgn, 2017-02-13 14:25:51]

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

[commit-bot: I haz the power, 2017-02-13 14:26: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, 2017-02-13 16:15:33]

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

[commit-bot: I haz the power, 2017-02-13 16:15:34]

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...)

[mdjones, 2017-02-13 17:40:37]

lgtm % nit

https://codereview.chromium.org/2670863004/diff/140001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
(right):

https://codereview.chromium.org/2670863004/diff/140001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java:21:
// TODO(https://crbug.com/691563): We need to force the ChromePreferenceManager
to
...ChromePreferenceManager to (initialize?) with the...

[gone, 2017-02-13 19:22:39]

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

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:279:
@SheetState
On 2017/02/13 14:25:39, dgn wrote:
> On 2017/02/10 18:17:08, dfalcantara (load balance plz) wrote:
> > I'd just put @SheetState on the same line to make it more obvious what it
goes
> > to.
> > 
> > @SheetState int targetState =
> >         getTargetSheetState(etc)
> 
> That's the style recommended by `git cl format`. It's also consistent with the
> other annotations across the codebase. At least in the files I usually look
at.
> I don't really want to change that if it starts polluting other people's CLs
> when the also use git cl format.

The problem is that git cl format doesn't work well in many, many cases.  This
is unfortunately one of them.  Also unfortunate is that git cl upload requires
this now.  Can you at least put a newline above the annotation so it's not an
annotation sandwich?

[gone, 2017-02-14 02:01:39]

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

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:279:
@SheetState
On 2017/02/13 19:22:39, dfalcantara (load balance plz) wrote:
> On 2017/02/13 14:25:39, dgn wrote:
> > On 2017/02/10 18:17:08, dfalcantara (load balance plz) wrote:
> > > I'd just put @SheetState on the same line to make it more obvious what it
> goes
> > > to.
> > > 
> > > @SheetState int targetState =
> > >         getTargetSheetState(etc)
> > 
> > That's the style recommended by `git cl format`. It's also consistent with
the
> > other annotations across the codebase. At least in the files I usually look
> at.
> > I don't really want to change that if it starts polluting other people's CLs
> > when the also use git cl format.
> 
> The problem is that git cl format doesn't work well in many, many cases.  This
> is unfortunately one of them.  Also unfortunate is that git cl upload requires
> this now.  Can you at least put a newline above the annotation so it's not an
> annotation sandwich?

Could you poke Nico Weber about this?  I'm not sure who to poke about weird git
cl formatting glitches.

[dgn, 2017-02-14 16:07:21]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-02-14 16:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mvanouwerkerk@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2670863004/#ps160001
(title: "rebase, properly fix test initialisation, address comments")

[dgn, 2017-02-14 16:07:40]

Thanks!

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

https://codereview.chromium.org/2670863004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:279:
@SheetState
On 2017/02/14 02:01:38, dfalcantara (load balance plz) wrote:
> On 2017/02/13 19:22:39, dfalcantara (load balance plz) wrote:
> > On 2017/02/13 14:25:39, dgn wrote:
> > > On 2017/02/10 18:17:08, dfalcantara (load balance plz) wrote:
> > > > I'd just put @SheetState on the same line to make it more obvious what
it
> > goes
> > > > to.
> > > > 
> > > > @SheetState int targetState =
> > > >         getTargetSheetState(etc)
> > > 
> > > That's the style recommended by `git cl format`. It's also consistent with
> the
> > > other annotations across the codebase. At least in the files I usually
look
> > at.
> > > I don't really want to change that if it starts polluting other people's
CLs
> > > when the also use git cl format.
> > 
> > The problem is that git cl format doesn't work well in many, many cases. 
This
> > is unfortunately one of them.  Also unfortunate is that git cl upload
requires
> > this now.  Can you at least put a newline above the annotation so it's not
an
> > annotation sandwich?
> 
Done.

> Could you poke Nico Weber about this?  I'm not sure who to poke about weird
git
> cl formatting glitches.

Done: https://crbug.com/692100

https://codereview.chromium.org/2670863004/diff/140001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
(right):

https://codereview.chromium.org/2670863004/diff/140001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java:21:
// TODO(https://crbug.com/691563): We need to force the ChromePreferenceManager
to
On 2017/02/13 17:40:37, mdjones wrote:
> ...ChromePreferenceManager to (initialize?) with the...

Done.

[commit-bot: I haz the power, 2017-02-14 16:07:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2017-02-14 16:08:01]

Description was changed from

==========
[Home] Add instrumentation test for Suggestions BottomSheet

Add a BottomSheetTestCaseBase to set up ChromeHome compatible test
cases. Add a test for issue 688391 to verify the touch interception
behaviour associated to the context menu manager.

BUG=688391
BUG=671361
==========

to

==========
[Home] Add instrumentation test for Suggestions BottomSheet

Add a BottomSheetTestCaseBase to set up ChromeHome compatible test
cases. Add a test for issue 688391 to verify the touch interception
behaviour associated to the context menu manager.

BUG=688391,671361
==========

[dgn, 2017-02-14 17:01:41]

The CQ bit was unchecked by dgn@chromium.org

[dgn, 2017-02-14 21:28:29]

The CQ bit was checked by dgn@chromium.org

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

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-14 22:21:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487107709067080,
"parent_rev": "9b27503cadb137969839573333b3a007d0a79eee", "commit_rev":
"a43c4f909bf3a4ee95a5b2739fe1175ff9080fd5"}

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

Description was changed from

==========
[Home] Add instrumentation test for Suggestions BottomSheet

Add a BottomSheetTestCaseBase to set up ChromeHome compatible test
cases. Add a test for issue 688391 to verify the touch interception
behaviour associated to the context menu manager.

BUG=688391,671361
==========

to

==========
[Home] Add instrumentation test for Suggestions BottomSheet

Add a BottomSheetTestCaseBase to set up ChromeHome compatible test
cases. Add a test for issue 688391 to verify the touch interception
behaviour associated to the context menu manager.

BUG=688391,671361

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

[commit-bot: I haz the power, 2017-02-14 22:22:23]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/a43c4f909bf3a4ee95a5b2739fe1...