Closed Bug 851861 Opened 11 years ago Closed 9 years ago

Intermittent testOverscroll, testPanCorrectness, testAxisLocking, testLoad, testFlingCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox24 disabled, firefox25 disabled, firefox26 disabled, firefox35 wontfix, firefox36 fixed, firefox37+ fixed, firefox38 fixed, firefox-esr31 wontfix)

Status:
RESOLVED FIXED
Milestone:
Firefox 38
Tracking Flags:
Tracking Status
firefox24 --- disabled
firefox25 --- disabled
firefox26 --- disabled
firefox35 --- wontfix
firefox36 --- fixed
firefox37 + fixed
firefox38 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: philor, Assigned: capella)

References

Details

(Keywords: intermittent-failure)

Attachments

(5 files, 1 obsolete file)

screenshot of Robocop Boxes
31.12 KB, image/png
Details
screenshot captured in recent failures
29.20 KB, image/jpeg
Details
bug851861.diff
10.32 KB, patch
kats
: review+
mfinkle
: approval-mozilla-aurora-
mfinkle
: approval-mozilla-beta-
Details | Diff | Splinter Review
bug851861_v1_backout.diff
9.41 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
bug851861_v2.diff
7.23 KB, patch
mfinkle
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=20731093&tree=Mozilla-Inbound
Android 4.0 Panda mozilla-inbound opt test robocop-1 on 2013-03-16 17:35:20 PDT for push 3648f5fa5787
slave: panda-0846

4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:55:54.414 I/Robocop ( 3716): 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:55:54.421 I/Robocop ( 3716): junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:55:54.429 I/Robocop ( 3716): 5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:55:55.656 I/TestRunner( 3716): junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)

https://tbpl.mozilla.org/php/getParsedLog.php?id=20736408&tree=Mozilla-Inbound
Android 4.0 Panda mozilla-inbound opt test robocop-1 on 2013-03-16 21:26:17 PDT for push 730fbb35edd5
slave: panda-0588

4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 21:42:38.351 I/Robocop ( 5744): 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 21:42:38.367 I/Robocop ( 5744): junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 21:42:38.367 I/Robocop ( 5744): 5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 21:42:39.625 I/TestRunner( 5744): junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)

https://tbpl.mozilla.org/php/getParsedLog.php?id=20730880&tree=Mozilla-Inbound
Android 4.0 Panda mozilla-inbound opt test robocop-1 on 2013-03-16 17:29:16 PDT for push 29fdd1949b09
slave: panda-0657

4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:51:24.796 I/Robocop ( 3443): 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:51:24.804 I/Robocop ( 3443): junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:51:24.804 I/Robocop ( 3443): 5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:51:26.078 I/TestRunner( 3443): junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:52:31.218 I/Robocop ( 3684): 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:52:31.226 I/Robocop ( 3684): junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:52:31.226 I/Robocop ( 3684): 5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
03-16 17:52:32.515 I/TestRunner( 3684): junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
https://tbpl.mozilla.org/php/getParsedLog.php?id=20737177&tree=Mozilla-Inbound
Summary: Intermittent testOverscroll, testPanCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0) → Intermittent testOverscroll, testPanCorrectness, testAxisLocking | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
https://tbpl.mozilla.org/php/getParsedLog.php?id=20751244&tree=Mozilla-Inbound
Summary: Intermittent testOverscroll, testPanCorrectness, testAxisLocking | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0) → Intermittent testOverscroll, testPanCorrectness, testAxisLocking, testLoad | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
https://tbpl.mozilla.org/php/getParsedLog.php?id=20782611&tree=Mozilla-Inbound
Summary: Intermittent testOverscroll, testPanCorrectness, testAxisLocking, testLoad | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0) → Intermittent testOverscroll, testPanCorrectness, testAxisLocking, testLoad, testFlingCorrectness | Pixel at 100, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(32,100,0)
The first instance of this I see is 

https://tbpl.mozilla.org/php/getParsedLog.php?id=20716358&tree=Mozilla-Inbound&full=1

from https://hg.mozilla.org/integration/mozilla-inbound/rev/60261728879e, Fri Mar 15 21:35:45 2013 PDT. 

This must be a regression from something landed on m-i on Mar 15, but there was so much bustage that day, it won't be easy to identify.
(In reply to Geoff Brown [:gbrown] from comment #9)
> This must be a regression from something landed on m-i on Mar 15, but there
> was so much bustage that day, it won't be easy to identify.

Not easy, but not impossible!

Blame lands on https://hg.mozilla.org/integration/mozilla-inbound/rev/a71c3f178d91.

Compare:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android&rev=a71c3f178d91
and
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android&rev=921e9f7f8924
I'll take a look at this - please let's not back this out unless we absolutely have to, this commit fixes a real bug that can be triggered regardless of the dynamic toolbar stuff (if you trigger the keyboard or rotate during startup, you can trigger it)

I expect these failures indicate there's still a way for compositor and Gecko window size to mismatch, but I'll need to look closer. How urgent is this, can we take this failure for a while? (I'm at a work week now and have a week of holiday after)
Adding needinfo? for comment #21.
Flags: needinfo?(philringnalda)
We could disable the tests for a couple of weeks, but there are at least 5 of them: testLoad, testOverscroll, testAxisLocking, testPanCorrectness, and testFlingCorrectness.
The way you get away with that is to take the bug, and then keep your head down and keep quiet, not broadcast "I plan on leaving this broken" as widely as possible.
Assignee: nobody → chrislord.net
Flags: needinfo?(philringnalda)
Depends on: 852526
It looks like this wasn't fixed by bug 852526, so I guess there's some race condition somewhere with setting the dynamic toolbar pref during robocop test startup(?)
(This is one of our more frequent oranges the last week)
(In reply to Chris Lord [:cwiiis] from comment #49)
> It looks like this wasn't fixed by bug 852526

I think it was fixed: Notice that Comment 48 is a slightly different error (rgba(0,101,33,255) vs rgba(0,0,0,255) makes it bug 836815) and that is the only failure since the landing in bug 852526.
Marking as fixed, wrt comment #51 - please reopen if not :)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Chris Lord [:cwiiis] from comment #52)
> Marking as fixed, wrt comment #51 - please reopen if not :)

ok
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I get this failure approximately 1 out of 10 times I run theOverscroll locally on an HTC Desire (Android 2.2). From what I see the block from page load is returned to fast and the "boxes" are not displayed yet when checking for the color. I also see a slowness sometimes to render content for the first page loaded after the test starts. Maybe we should just load a Robocop Blank page before this one.
After doing some investigations on this adding a loadUrl of the Robocop blank page first and then Robocop boxes seems to solve the failure of the first check. Unfortunately at least on my Desire (Android 2.2) the dragScroll from testPanCorrectness and testAxisLock is always 6 pixels short although it's done over 100 pixels. Adding 6 pixels to the drag scrolls correctly but i still get a fail for the color checks from (100,100) to (200,200) - the last part of the checks - because for some reason it checks the color at 0,0 and gets(0,0,0) instead of (32.200.32).
Attached image Screenshot htc desire Z (obsolete) — 
Looking into this locally on an HTC Desire Z (Android 2.3.3) the algorithm for the boxes seems off. The first row seems wider then the second - please see the attached screenshot. I will investigate further.
The difference is more visible in this screenshot
Attachment #777007 - Attachment is obsolete: true
Printing every pixel in Robocop boxes it seems that the first box with rgb(0,0,0,255) is 149 pixels high and 103 wide. The rest of the boxes continue similarly with widths and heights that are off from the defined 100 px high and 100 px wide.
(In reply to Adrian Tamas from comment #130)
> Printing every pixel in Robocop boxes it seems that the first box with
> rgb(0,0,0,255) is 149 pixels high and 103 wide. 

That does not seem consistent with the screen shot: each box in the screenshot looks roughly square to me.

Another technique that may be instructive is to collect the file /mnt/sdcard/tests/pixels.map and analyze that.
It may be a visual illusion because of the boxes but the second row looks narrower to me and especially on a device.
:cwiiis -- Do you know what's going wrong here? Are you making any progress? This is one of the most frequent test failures on tbpl currently.
Flags: needinfo?(chrislord.net)
(In reply to Geoff Brown [:gbrown] from comment #159)
> :cwiiis -- Do you know what's going wrong here? Are you making any progress?
> This is one of the most frequent test failures on tbpl currently.

sorry, I forgot this was assigned to me... I haven't made any progress and I only have vague ideas as to what this could be.

I could use some help here, I don't know if I really have time for this. I could certainly help guide someone through root-causing it.
Flags: needinfo?(chrislord.net)
Changes in bug 898036, landing today, may affect the behavior here.  I expect the intermittent failures to continue, but the screenshot and experience related by :AdrianT in comments 129+ will likely change.
Some recent observations:
 - all the recent failures are on Panda / Android 4.0
 - all the recent failures are "Color rgba(0,0,0,255) not close enough"
 - I reproduced in a try run and printed all the pixels in the vicinity -- they were all 0,0,0 -- did the page not load? https://tbpl.mozilla.org/php/getParsedLog.php?id=25807691&tree=Try&full=1#error0
 - I see an EGL error during every failed load, typically "eglMakeCurrent:696 error 300d (EGL_BAD_SURFACE)" and sometimes "eglMakeCurrent:651 error 3009 (EGL_BAD_MATCH)"

Also, I stumbled on this peculiar result: Sleeping for 5 additional seconds before loading the page appears to make the tests run reliably: https://tbpl.mozilla.org/?tree=Try&rev=2c1535e75e64
(In reply to Geoff Brown [:gbrown] from comment #234)
> Some recent observations:
>  - all the recent failures are on Panda / Android 4.0
>  - all the recent failures are "Color rgba(0,0,0,255) not close enough"
>  - I reproduced in a try run and printed all the pixels in the vicinity --
> they were all 0,0,0 -- did the page not load?
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=25807691&tree=Try&full=1#error0
>  - I see an EGL error during every failed load, typically
> "eglMakeCurrent:696 error 300d (EGL_BAD_SURFACE)" and sometimes
> "eglMakeCurrent:651 error 3009 (EGL_BAD_MATCH)"
> 
> Also, I stumbled on this peculiar result: Sleeping for 5 additional seconds
> before loading the page appears to make the tests run reliably:
> https://tbpl.mozilla.org/?tree=Try&rev=2c1535e75e64

I haven't tried this yet but I was thinking that we could add a mInstrumentation.waitForIdleSync() before returning from BlockForGeckoReady since there are still a lot of other things that take up processing time after Gecko:Ready and the app can still be slow after returning
Tests disabled (testOverscroll, testPanCorrectness, testAxisLocking, testLoad & testFlingCorrectness) for too many intermittent failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/406668176382
Whiteboard: [test disabled][leave open]
Flags: needinfo?(ryanvm)
https://tbpl.mozilla.org/?tree=Try&rev=eba988dc22c5 suggests that these tests are working now. I don't know what changed. 

Any objection to enabling

    [testLoad]
    [testPanCorrectness]
    [testFlingCorrectness]
    [testOverscroll]
    [testAxisLocking]

?
Flags: needinfo?(emorley)
Look good to me, thank you :-)
Flags: needinfo?(emorley)
Enabling:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3046b6eb790c
Whiteboard: [test disabled][leave open] → [leave open]
From comment 342, it would appear that this is fixed now? Resolving as worksforme.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Is this still an issue or can we close this? Either way, unassigning.
Assignee: chrislord.net → nobody
Flags: needinfo?(emorley)
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(emorley)
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
:kats -- Any ideas here?

I find it interesting that recent failures occur in several (all?) of the pixel tests and all seem to find rgba(0,176,144,255) at (0,0).

I cannot reproduce locally.
Flags: needinfo?(bugmail.mozilla)
(In reply to Geoff Brown [:gbrown] from comment #423)
> :kats -- Any ideas here?
> 
> I find it interesting that recent failures occur in several (all?) of the
> pixel tests and all seem to find rgba(0,176,144,255) at (0,0).

That is kind of interesting. It seems to have started at comment 353. I couldn't find any color close to rgb(0,176,144) on the boxes page, so I'm not sure where that's coming from. If we can get a video or screenshot of these test at the point of failure then that might provide some insight. I also quickly looked for recent changes that might have affected the initial viewport of the page but didn't see anything that landed at around when this changed.
Flags: needinfo?(bugmail.mozilla)
There is a screenshot captured by recent failures. It seems to show the text selection action bar. There should not be any text on this page...how might this be happening?
Flags: needinfo?(wjohnston)
Geoff, I don't suppose you could find someone to take a look at this top orange. Will need to disable sooner otherwise - thanks :-)
Flags: needinfo?(gbrown)
Keywords: leave-open
Whiteboard: [leave open] → [disableme 2014-08-25]
:wesj will take a look.
Assignee: nobody → wjohnston
Flags: needinfo?(wjohnston)
Flags: needinfo?(gbrown)
Recent failures continue to generate the screenshot I noted in Comment 449 (text selection action bar).
Wes, according to comment 506, you've been looking for nearly 4 months now? Should I just start disabling?
Flags: needinfo?(wjohnston)
[Tracking Requested - why for this release]:

I didn't realize this was on my plate either. I think disabling these would probably be bad. Can we send this over to the Fennec drivers for them to evaluate before you do?
tracking-firefox37: --- → ?
Flags: needinfo?(wjohnston)
Attached patch bug851861.diff — — Splinter Review 
I think the patch in bug 1119834 will help this, however, the issue of accidentally triggering longpress (vs responding in a better fashion) could be addressed.

In bug 1119834 comment #8 kats mentioned a couple approaches. I wonder if this patch, (or some minor variation of method invocation) would be as appropriate / simple.

Quick push to try [0], shows solution in action [1] ... search |onLongPress() Triggered| followed by |Longpress Event is ignored|.


[0] https://tbpl.mozilla.org/?tree=Try&rev=539d4156876e
[1] http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/97b33fe3b150df7aaa68c590d4e95d8940c57e6e6cbe48a064ee7133de3324b02c559458d1695744ec47cbfa04f5f7f1fc7941ef29af002c363f1e3a681b57e1
Attachment #8549795 - Flags: review?(bugmail.mozilla)
Comment on attachment 8549795 [details] [diff] [review]
bug851861.diff

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

This approach seems fine to me, thanks!

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +1349,5 @@
>      }
>  
>      @Override
>      public void onLongPress(MotionEvent motionEvent) {
> +        Log.i(LOGTAG, "onLongPress() Triggered");

Remove this log before landing
Attachment #8549795 - Flags: review?(bugmail.mozilla) → review+
I think MotionEventReplayer (used by testCheck2) has the potential for the same issue. Can you handle that case also?
https://hg.mozilla.org/integration/fx-team/rev/aa29e86d1f0e
Assignee: wjohnston → markcapella
Status: REOPENED → ASSIGNED
gbrown: re: testCheck2 ... I didn't address |testCheck|'s here as they seem disabled in robocop.ini. 

Also, I didn't see concrete examples of fails, or open bugzilla's referencing the test. I took a quick look at the input events contained in assets/testcheck2-motionevents, and yikes / voluminous.

Can you open a seperate-scope bug for MotionEventReplayer and assign it to me? I can run some tests and based on the results, we can go from there.
Comment on attachment 8549795 [details] [diff] [review]
bug851861.diff

Approval Request Comment - Fixes an old random orange plaguing sheriffs fingers

[Feature/regressing bug #]: No regression, shortcoming in initial process
[User impact if declined]: No user impact, annoys sherrifs
[Describe test coverage new/current, TBPL]: Currently in m-c and seems to be working

[Risks and why]: Adds code toggle to mobile/browser.js so test infra can disable response to longpress events auto-triggered by underlying Android OS

[String/UUID change made/needed]: none
Attachment #8549795 - Flags: approval-mozilla-beta?
Attachment #8549795 - Flags: approval-mozilla-aurora?
Attachment #8549795 - Flags: approval-mozilla-beta?
Attachment #8549795 - Flags: approval-mozilla-beta+
Attachment #8549795 - Flags: approval-mozilla-aurora?
Attachment #8549795 - Flags: approval-mozilla-aurora+
Comment on attachment 8549795 [details] [diff] [review]
bug851861.diff


>diff --git a/mobile/android/base/gfx/JavaPanZoomController.java b/mobile/android/base/gfx/JavaPanZoomController.java

>     @Override
>     public void onLongPress(MotionEvent motionEvent) {
>+        Log.i(LOGTAG, "onLongPress() Triggered");

Would be nice to not leave this debugging Log in the code

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   contextmenus: {
>     items: {}, //  a list of context menu items that we may show
>     DEFAULT_HTML5_ORDER: -1, // Sort order for HTML5 context menu items
>+    _isLongPressEnabled: 1, // Android longpress events can be ignored during robocop tests.
> 
>     init: function() {
>       BrowserApp.deck.addEventListener("contextmenu", this.show.bind(this), false);
>+
>+      Messaging.addListener((data) => {
>+        return {result: (this._isLongPressEnabled = data.isLongPressEnabled)};
>+      }, "ContextMenu:SetIsLongpressEnabled");

I don't like having our test infra force us to put code in various places like this. It's pretty fragile.

>     show: function(event) {

>       // Android Long-press / contextmenu event provides clientX/Y data. This is not provided
>       // by mochitest: test_browserElement_inproc_ContextmenuEvents.html.
>       if (!event.clientX || !event.clientY) {
>         return;
>       }

More test-specific code crept in?
(In reply to Mark Finkle (:mfinkle) from comment #1099)
> Comment on attachment 8549795 [details] [diff] [review]
> bug851861.diff
> 
> 
> >diff --git a/mobile/android/base/gfx/JavaPanZoomController.java b/mobile/android/base/gfx/JavaPanZoomController.java
> 
> >     @Override
> >     public void onLongPress(MotionEvent motionEvent) {
> >+        Log.i(LOGTAG, "onLongPress() Triggered");
> 
> Would be nice to not leave this debugging Log in the code

My mistake. This is not in the patch that landed.

> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >   contextmenus: {
> >     items: {}, //  a list of context menu items that we may show
> >     DEFAULT_HTML5_ORDER: -1, // Sort order for HTML5 context menu items
> >+    _isLongPressEnabled: 1, // Android longpress events can be ignored during robocop tests.
> > 
> >     init: function() {
> >       BrowserApp.deck.addEventListener("contextmenu", this.show.bind(this), false);
> >+
> >+      Messaging.addListener((data) => {
> >+        return {result: (this._isLongPressEnabled = data.isLongPressEnabled)};
> >+      }, "ContextMenu:SetIsLongpressEnabled");
> 
> I don't like having our test infra force us to put code in various places
> like this. It's pretty fragile.

Mark and I chatted on IRC and maybe we could remove this code and add a method in Java to force Java to stop sending long-taps to Gecko. That way we could remove the browser.js code.
Comment on attachment 8549795 [details] [diff] [review]
bug851861.diff

Let's hold off on the uplift until we get a different patch
Attachment #8549795 - Flags: approval-mozilla-beta-
Attachment #8549795 - Flags: approval-mozilla-beta+
Attachment #8549795 - Flags: approval-mozilla-aurora-
Attachment #8549795 - Flags: approval-mozilla-aurora+
First I'd backout the previous patch, since we're pretty much replacing it.
Attachment #8551976 - Flags: review?(mark.finkle)
The slightly more streamlined version ... avoids browser.js
Attachment #8551977 - Flags: review?(mark.finkle)
Comment on attachment 8551977 [details] [diff] [review]
bug851861_v2.diff

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

::: mobile/android/base/gfx/LayerView.java
@@ +142,5 @@
> +     * MotionEventHelper dragAsync() robocop tests can instruct
> +     * PanZoomController not to generate longpress events.
> +     */
> +    public void setIsLongpressEnabled(boolean isLongpressEnabled) {
> +        ((JavaPanZoomController) mPanZoomController).setIsLongpressEnabled(isLongpressEnabled);

This will make it harder to fix bug 776030. Not that anybody is actively working on it, but just sayin'.
pushed to try, hammered on the tests
https://tbpl.mozilla.org/?tree=Try&rev=b272361c487

notes: testVkbOverlap shares code path here, but is disabled due to # bug 907274.
       also: bug 1085609 seems to need some love.
Attachment #8551976 - Flags: review?(mark.finkle) → review+
Comment on attachment 8551977 [details] [diff] [review]
bug851861_v2.diff

We might be able to eventually move this into the Gecko c++ code and use a Gecko pref to drive it. This is a fine approach for now.

Yes, this will be "one more thing" to deal with when converting to APZC, but I think that switch will have a big impact on testing anyway, creating new problems and fixing old problems.
Attachment #8551977 - Flags: review?(mark.finkle) → review+
Comment on attachment 8551977 [details] [diff] [review]
bug851861_v2.diff

Approval Request Comment: newer version of the original patch ... same solution to the issue, just approached / coded slightly differently.

[Feature/regressing bug #]: No regression, shortcoming in initial process
[User impact if declined]: No user impact, annoys sherrifs
[Describe test coverage new/current, TBPL]: Currently in m-c and seems to be working

[Risks and why]: Adds code toggle to JavaPanZoomController so test infra can disable response to longpress events auto-triggered by underlying Android OS during lengthy drag events.

[String/UUID change made/needed]: none
Attachment #8551977 - Flags: approval-mozilla-beta?
Attachment #8551977 - Flags: approval-mozilla-aurora?
Attachment #8551977 - Flags: approval-mozilla-beta?
Attachment #8551977 - Flags: approval-mozilla-beta+
Attachment #8551977 - Flags: approval-mozilla-aurora?
Attachment #8551977 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2869d00dc482
https://hg.mozilla.org/releases/mozilla-beta/rev/3aca4622bfd5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 9 years ago
status-firefox35: --- → wontfix
status-firefox36: affected → fixed
status-firefox37: affected → fixed
status-firefox38: --- → fixed
status-firefox-esr31: --- → wontfix
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [disableme 2014-08-25]
Target Milestone: --- → Firefox 38
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: