Closed Bug 996457 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: alessarik, Assigned: alessarik)

Details

Attachments

(1 file, 3 obsolete files)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
+ Change test LongPressPreventDefault to function DoLongPressPreventDefaultTest with set AllowedTouchBehavior from param.
+ Create two tests LongPressPreventDefault and LongPressPreventDefaultPanAndZoom
Attachment #8407510 - Flags: review?(mbrubeck)
Attachment #8407510 - Flags: review?(mak77)
Attachment #8407510 - Flags: review?(gavin.sharp)
Attachment #8407510 - Flags: review?(bugs)
Attachment #8407510 - Flags: review?(bugmail.mozilla)
Attachment #8407510 - Flags: feedback?(oleg.romashin)
Attachment #8407510 - Flags: feedback?(nicklebedev37)
Attachment #8407510 - Flags: review?(mbrubeck)
Attachment #8407510 - Flags: review?(mak77)
Attachment #8407510 - Flags: review?(gavin.sharp)
Attachment #8407510 - Flags: review?(bugs)
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Can you explain why the test was failing and how your change fixes it?
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.
Attachment #8407510 - Flags: review?(bugmail.mozilla) → review-
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().
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
+ changes according with comments
Attachment #8407510 - Attachment is obsolete: true
Attachment #8407510 - Flags: feedback?(oleg.romashin)
Attachment #8407510 - Flags: feedback?(nicklebedev37)
Attachment #8408971 - Flags: review?(bugmail.mozilla)
Attachment #8408971 - Flags: review?(botond)
Attachment #8408971 - Flags: feedback?(oleg.romashin)
Attachment #8408971 - Flags: feedback?(nicklebedev37)
Flags: needinfo?(alessarik)
Looks like comments from Botond resolve second issue.
https://tbpl.mozilla.org/?tree=Try&rev=350e3a50d428
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.
Attachment #8408971 - Flags: feedback?(nicklebedev37) → feedback+
Attachment #8408971 - Flags: review?(botond) → review+
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.
Attachment #8408971 - Flags: review?(bugmail.mozilla) → review-
+ update names of tests
Attachment #8408971 - Attachment is obsolete: true
Attachment #8408971 - Flags: feedback?(oleg.romashin)
Attachment #8409553 - Flags: review?(bugmail.mozilla)
Attachment #8409553 - Flags: feedback?(oleg.romashin)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
+ update commit message
Attachment #8409553 - Attachment is obsolete: true
Attachment #8409553 - Flags: review?(bugmail.mozilla)
Attachment #8409553 - Flags: feedback?(oleg.romashin)
Attachment #8409572 - Flags: review?(bugmail.mozilla)
Attachment #8409572 - Flags: feedback?(oleg.romashin)
Comment on attachment 8409572 [details] [diff] [review]
repair_test_996457_LongPressPreventDefault_ver3.diff

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

This is good, thanks.
Attachment #8409572 - Flags: review?(bugmail.mozilla) → review+
Can I checkin this version of patch?
Yeah, that's fine by me. I don't know if you want to wait for the feedback by romaxa since you requested it.
Attachment #8409572 - Flags: feedback?(oleg.romashin) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dcc3ce7fcd23
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.