737411 - Robocop: SoftwareLayerClient vs LayerClient and GLSurfaceView use

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

we use reflection in robocop to call internal gecko methods:
http://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/FennecNativeActions.java.in#244

we need to remove the s/Software// to get things running.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: simple name change

Comment 2

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

Comment on attachment 607640 [details] [diff] [review]
simple name change

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

You should also:
1) Add comments to the methods/classes referenced via reflection so that this doesn't happen again. There are already comments all over the file that say things like "/** This function is invoked by Gecko via JNI; be careful when modifying signature. */" - do something similar for reflection.
2) Ensure that exceptions that result from this cause the test to fail. Tests that pass on tbpl but aren't testing anything because the exceptions get caught are worse than not having tests at all.

Comment 3

[Geoff Brown [:gbrown]]

After making this change, I tried to test using testAxisLocking. The original problem was resolved, but another similar issue was found: FennecNativeDriver.getSurfaceView was searching for an instance of GLSurfaceView, which is no longer used.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: updated patch

This patch updates out-dated references to SoftwareLayerClient and GLSurfaceView. 

I have added "be careful/reflection" comments to several methods -- will they do any good? 

I have also added explicit checks for getPaintedSurface failure. The more general issue of Robocop not noticing exceptions / failures is being handled by jmaher in other bugs.


Unfortunately, with this patch, all PixelTests fail for me with:

Pixel at 0, 0 - Color rgba(0,0,0,255) not close enough to expected rgb(0,255,255)

The pixels.map file seems to contain 00ff0000 in every word - maybe getPixels() is also broken?

Comment 5

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

Comment on attachment 607685 [details] [diff] [review]
updated patch

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

r+ with typo fixed

::: mobile/android/base/gfx/LayerView.java
@@ +62,5 @@
>   *
>   * This view delegates to LayerRenderer to actually do the drawing. Its role is largely that of a
>   * mediator between the LayerRenderer and the LayerController.
> + *
> + * Note that LayerView is accessed by Robocop vi reflection.

s/vi/via/

Comment 6

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

(In reply to Geoff Brown [:gbrown] from comment #4)
> I have added "be careful/reflection" comments to several methods -- will
> they do any good? 

Looks good. People might still change these functions, but at least they have a better chance of also fixing the associated robocop code.

> 
> I have also added explicit checks for getPaintedSurface failure. The more
> general issue of Robocop not noticing exceptions / failures is being handled
> by jmaher in other bugs.
> 

ok.

> 
> Unfortunately, with this patch, all PixelTests fail for me with:
> 
> Pixel at 0, 0 - Color rgba(0,0,0,255) not close enough to expected
> rgb(0,255,255)
> 
> The pixels.map file seems to contain 00ff0000 in every word - maybe
> getPixels() is also broken?

Yeah, it's quite possible. That code has changed recently, in particular with bug 731603 which just landed yesterday. Robocop was passing when I landed it, but since robocop was already broken it may have not picked up breakage. I can take a look at this; hold off landing this patch until I at least figure out if it's an easy fix or if we should just disable the tests for now.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

if we can make a decision today or first thing tomorrow on fixing/disabling the tests, then we can get this patch landed so we can move forward with talos + robocop.

Comment 8

[Geoff Brown [:gbrown]]

Attachment: updated patch with typo fixed

Typo fix only. r=kats

Comment 9

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

Bug 737630 may have caused the getPixels() breakage; I just landed a patch to fix it. If you're still seeing getPixels() returning garbage with that patch applied then feel free to disable the tests for now, and assign me a new bug to get them fixed.

Comment 10

[Geoff Brown [:gbrown]]

https://tbpl.mozilla.org/?tree=Try&rev=979191796ea1 confirms PixelTest failures, as reported in Comment 4. (Patch from bug 737630 was NOT yet applied - checking that tonight.)

Comment 11

[Geoff Brown [:gbrown]]

Bug 737630 definitely makes a difference, but PixelTest-based tests still fail with that patch. Now the pixels.map has varied, box-like data for the boxes tests, some pixel checks pass, but others fail:

Pixel at 100, 0 - Color rgba(99,255,255,255) not close enough to expected rgb(100,255,245)

Comment 12

[Geoff Brown [:gbrown]]

https://tbpl.mozilla.org/?tree=Try&rev=b12a718cb1f1 confirms continued PixelTest failures with patch from Bug 737630.

Comment 13

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

Attachment: Fix pixel tests

Applying this patch on top of the other one makes all the tests (that are currently enabled) pass locally for me.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 608342 [details] [diff] [review]
Fix pixel tests

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

this set of changes works locally for me.  I am still confused by the math conversion.  Can we put a link/comment somewhere that explains the formula for the RGB888/555 conversion?

Comment 15

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

Attachment: Fix pixel tests (v2)

Updated comment in robocop_boxes with some more info about the colors. Carrying r+

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/89b83fc4fdb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b6456d96d1

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/89b83fc4fdb0
https://hg.mozilla.org/mozilla-central/rev/86b6456d96d1