996457 - TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.LongPressPreventDefault | Actual function call count doesn't match EXPECT_CALL(*mcc, HandleLongTapUp(CSSPoint(touchX, touchEndY), 0, apzc->GetGuid()))...

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Maksim Lebedev]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Gtest on try servers


Actual results:

TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.LongPressPreventDefault | Actual function call count doesn't match EXPECT_CALL(*mcc, HandleLongTapUp(CSSPoint(touchX, touchEndY), 0, apzc->GetGuid()))...


Expected results:

Expected: to be called once

Gtest have to be pass.

Comment 1

[Oleg Romashin (MS)]

I beleive you forgot to mention that Steps to reproduce not just try servers, default try servers works fine https://tbpl.mozilla.org/

you did apply patch which does enable touch action preference by default, and that is the real reason:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37506653&tree=Try#error0

Comment 2

[Maksim Lebedev]

Attachment: repair_test_996457_LongPressPreventDefault.diff

+ Change test LongPressPreventDefault to function DoLongPressPreventDefaultTest with set AllowedTouchBehavior from param.
+ Create two tests LongPressPreventDefault and LongPressPreventDefaultPanAndZoom

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Can you explain why the test was failing and how your change fixes it?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8407510 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault.diff

Review of attachment 8407510 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this takes into account the changes made as part of bug 976605 (see patch 13 on that bug, and comment 70). You should update your code and adjust your code accordingly.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8407510 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault.diff

Review of attachment 8407510 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +819,5 @@
>    apzc->SetFrameMetrics(TestFrameMetrics());
>    apzc->NotifyLayersUpdated(TestFrameMetrics(), true);
>    apzc->UpdateZoomConstraints(ZoomConstraints(false, false, CSSToScreenScale(1.0), CSSToScreenScale(1.0)));
>  
> +  nsTArray<uint32_t> values;

Please call this something more descriptive, like 'allowedTouchBehaviors'.

@@ +822,5 @@
>  
> +  nsTArray<uint32_t> values;
> +  values.AppendElement(aBehavior);
> +  apzc->SetTouchActionEnabled(aShouldUseTouchAction);
> +  apzc->SetAllowedTouchBehavior(values);

As Kats pointed out, since bug 976605 SetAllowedTouchBehavior() needs to be called after the touch-start is sent. In other words, do it after the ApzcDown().

Comment 6

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Can you explain why the test was failing and how your change fixes it?
Yes. Please see stack:
>TEST_F(AsyncPanZoomControllerTester, LongPressPreventDefault)
>apzc->ContentReceivedTouch(false)
>AsyncPanZoomController::CheckContentResponse
>{
>  if (mTouchActionPropertyEnabled) {
>    canProceedToTouchState &= mTouchBlockState.mAllowedTouchBehaviorSet;
>  }
>  if (!canProceedToTouchState) {
>    return;
>  }
>}
As result we have fails test.
Please see evidence of my idea in 
https://tbpl.mozilla.org/php/getParsedLog.php?id=37928948&tree=Try#error0
LongPressPreventDefault test was completed succesfully.
Unfortunately, second LongPressPreventDefaultPanAndZoom test was failed.
I will investigate this issue.

Comment 7

[Maksim Lebedev]

Attachment: repair_test_996457_LongPressPreventDefault_ver2.diff

+ changes according with comments

Comment 8

[Maksim Lebedev]

Looks like comments from Botond resolve second issue.
https://tbpl.mozilla.org/?tree=Try&rev=350e3a50d428

Comment 9

[Nick Lebedev [:nl]]

Comment on attachment 8408971 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault_ver2.diff

Review of attachment 8408971 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good,
the only thing i would suggest to change is to rename LongPressPreventDefaultPanAndZoom to something more related to touch-action, e.g. LongPressPreventDefaultWithTouchAction. Possibly we should have done it for LongPressPanAndZoom too.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8408971 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault_ver2.diff

Review of attachment 8408971 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Maksim Lebedev from comment #6)
> Yes. Please see stack:
> >TEST_F(AsyncPanZoomControllerTester, LongPressPreventDefault)
> >apzc->ContentReceivedTouch(false)
> >AsyncPanZoomController::CheckContentResponse
> >{
> >  if (mTouchActionPropertyEnabled) {
> >    canProceedToTouchState &= mTouchBlockState.mAllowedTouchBehaviorSet;
> >  }
> >  if (!canProceedToTouchState) {
> >    return;
> >  }
> >}
> As result we have fails test.
> Please see evidence of my idea in 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37928948&tree=Try#error0
> LongPressPreventDefault test was completed succesfully.
> Unfortunately, second LongPressPreventDefaultPanAndZoom test was failed.
> I will investigate this issue.

This isn't really an explanation of what's going on. Even though it's obvious from the patch in this case, your commit message needs to describe what the patch is doing, and the current commit message is inadequate. r- for that. In general please also explain what you're doing so that reviewers have an easier time reviewing the patches, and people who need to understand why the change was made later can figure it out.

Also I agree with Nick that the test should be renamed to refer to touch action.

Comment 11

[Maksim Lebedev]

Attachment: repair_test_996457_LongPressPreventDefault_ver3.diff

+ update names of tests

Comment 12

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> This isn't really an explanation of what's going on. Even though it's
> obvious from the patch in this case, your commit message needs to describe
> what the patch is doing, and the current commit message is inadequate. r-
> for that. In general please also explain what you're doing so that reviewers
> have an easier time reviewing the patches, and people who need to understand
> why the change was made later can figure it out.
Unfortunately, I cannot write poem on English, but I can to show issue in this test.
>void AsyncPanZoomController::CheckContentResponse() {
>  bool canProceedToTouchState = true;
>  if (mFrameMetrics.mMayHaveTouchListeners) {
>    canProceedToTouchState &= mTouchBlockState.mPreventDefaultSet;
>  }
>  if (mTouchActionPropertyEnabled) {
>    canProceedToTouchState &= mTouchBlockState.mAllowedTouchBehaviorSet;
>  }
>  if (!canProceedToTouchState) {
>    return;
>  }
When we have mTouchActionPropertyEnabled as true we should have mTouchBlockState.mAllowedTouchBehaviorSet as true. Otherwise canProceedToTouchState will be false,
and we only finish this function and all other things will not be done.
We had the same issue in bug 973660
That's why we need to call SetAllowedTouchBehavior in test.

Comment 13

[Maksim Lebedev]

Attachment: repair_test_996457_LongPressPreventDefault_ver3.diff

+ update commit message

Comment 14

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=c2938966f985

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8409572 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault_ver3.diff

Review of attachment 8409572 [details] [diff] [review]:
-----------------------------------------------------------------

This is good, thanks.

Comment 16

[Maksim Lebedev]

Can I checkin this version of patch?

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Yeah, that's fine by me. I don't know if you want to wait for the feedback by romaxa since you requested it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc3ce7fcd23

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcc3ce7fcd23