Last Comment Bug 702416 - [layers] Replace homebrew classes with Android ones
: [layers] Replace homebrew classes with Android ones
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 normal (vote)
: ---
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-14 13:27 PST by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-01-09 10:56 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
(1/4) Replace intpoint (8.99 KB, patch)
2011-11-14 13:30 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review
(2/4) Replace floatpoint (22.54 KB, patch)
2011-11-14 13:31 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review
(3/4) Replace IntRect (28.15 KB, patch)
2011-11-14 13:32 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review
(4/4) Replace FloatRect (37.04 KB, patch)
2011-11-14 13:32 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:27:56 PST
The IntPoint, FloatPoint, IntRect, and FloatRect classes should be replaced with the Android-provided Point, PointF, Rect, and RectF ones respectively.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:30:51 PST
Created attachment 574397 [details] [diff] [review]
(1/4) Replace intpoint
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:31:21 PST
Created attachment 574398 [details] [diff] [review]
(2/4) Replace floatpoint
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:32:31 PST
Created attachment 574399 [details] [diff] [review]
(3/4) Replace IntRect
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:32:59 PST
Created attachment 574400 [details] [diff] [review]
(4/4) Replace FloatRect
Comment 5 Patrick Walton (:pcwalton) 2011-11-14 13:44:30 PST
Comment on attachment 574397 [details] [diff] [review]
(1/4) Replace intpoint

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

LGTM
Comment 6 Patrick Walton (:pcwalton) 2011-11-14 13:46:13 PST
Comment on attachment 574398 [details] [diff] [review]
(2/4) Replace floatpoint

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

r+
Comment 7 Patrick Walton (:pcwalton) 2011-11-14 13:50:02 PST
Comment on attachment 574399 [details] [diff] [review]
(3/4) Replace IntRect

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

r=me with comment

::: embedding/android/GeckoApp.java
@@ -748,4 +748,4 @@
> >                  });
> >                  connectGeckoLayerClient();
> >              } else if (event.equals("PanZoom:Ack")) {
> > -                final IntRect rect = new IntRect(message.getJSONObject("rect"));
> > +                Rect rect = RectUtils.create(message.getJSONObject("rect"));

Standard Java style prefers "createInstance", but that's gratuitously long, so let's leave it as create.

::: embedding/android/gfx/TileLayer.java
@@ +58,5 @@
>      private boolean mRepeat;
>      private IntSize mSize;
>      private int[] mTextureIDs;
>  
> +    private Rect mTextureUploadRect;

I'd add a comment here saying that it's not safe to hand this rect out, since it may be mutated (via the union line below) and that would surprise callers.
Comment 8 Patrick Walton (:pcwalton) 2011-11-14 13:55:05 PST
Comment on attachment 574400 [details] [diff] [review]
(4/4) Replace FloatRect

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

r=me

::: embedding/android/ui/ViewportController.java
@@ +92,2 @@
>          float zoomFactor = (float)layerPageSize.width / (float)mPageSize.width;
> +        return RectUtils.scale(layerVisibleRect, 1.0f / zoomFactor);

scaleAll() adjusts X and Y as well as width and height. (Note that I'm going to be throwing a lot of this code away, so this doesn't really matter.)

Note You need to log in before you can comment on or make changes to this bug.