A few nits and small improvements in ZoomedView.java

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: domivinc, Assigned: domivinc)

Tracking

Trunk
Firefox 38
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Followup of bug 663803 (from comment 139 https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c139)

Kats:
The code seems pretty sane to me and I don't see any major problems. However there are quite a few nits and small improvements that could be made; see my comments below. Up to you which ones you want to do and which ones you want to ignore - I haven't worked in this code for a while so I'm probably out of date with respect to what the current conventions are. You already have two r+'s so you don't need mine as well, but giving an f+ anyway to indicate I've looked over it and it looks good.

::: mobile/android/base/BrowserApp.java
@@ +959,5 @@
>          ThreadUtils.assertOnUiThread();
>  
>          if (enabled) {
>              if (mLayerView != null) {
> +                mLayerView.setOnMetricsChangedDynamicToolbarViewportListener(this);


Rather than having OnMetricsChangedDynamicToolbarViewportListener and OnMetricsChangedZoomedViewportListener I would prefer you just had "addMetricsChangedListener" and "removeMetricsChangedListener" and store a List<LayerView.OnMetricsChangedListener> in GeckoLayerClient.

::: mobile/android/base/ZoomedView.java
@@ +1,2 @@
> +package org.mozilla.gecko;
> +


This file needs a copyright header thingy. And many of the functions in this file can be private, please reduce the visibility wherever you can. That way you can also remove some of the layerView==null checks which will become redundant. For example all the call sites of setCapturedSize already check for a null layerView or enforce that it is non-null, so that function (once private) doesn't need to do the check again.

@@ +1,5 @@
> +package org.mozilla.gecko;
> +
> +import java.text.DecimalFormat;
> +
> +import java.nio.ByteBuffer;


Organize imports by blocks separated by empty line: org.mozilla.*, com.*, net.*, org.*, android.*, then java.*

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Java_practices

@@ +29,5 @@
> +import android.widget.RelativeLayout;
> +
> +public class ZoomedView extends FrameLayout implements LayerView.OnMetricsChangedListener,
> +        LayerView.OnZoomedViewListener, GeckoEventListener {
> +    private static final String LOGTAG = "Gecko" + ZoomedView.class.getSimpleName();


I assume this getSimpleName business is some new fennec convention since I see it in a few files. Silly, if you ask me :)

@@ +39,5 @@
> +    private static final int DELAY_BEFORE_NEXT_RENDER_REQUEST_MS = 2000;
> +
> +    private ImageView zoomedImageView;
> +    private LayerView layerView;
> +    private MotionEvent actionDownEvent;


Move actionDownEvent to be inside ZoomedViewTouchListener, you don't see to use it outside that anywhere.

@@ +50,5 @@
> +    private PointF returnValue;
> +
> +    private boolean stopUpdateView;
> +
> +    private int lastOrientation = 0;


Class members get initialized to 0 by default, so no need for explicit initialization here.

@@ +55,5 @@
> +
> +    private ByteBuffer buffer;
> +    private Runnable requestRenderRunnable;
> +    private long startTimeReRender = 0;
> +    private long lastStartTimeReRender = 0;


Ditto

@@ +60,5 @@
> +
> +    private class ZoomedViewTouchListener implements View.OnTouchListener {
> +        private float originRawX;
> +        private float originRawY;
> +        private int touchState;


Instead of an int I would just make this a "bool dragged" or something. It only ever has two values anyway.

@@ +106,5 @@
> +
> +        private boolean moveZoomedView(MotionEvent event) {
> +            RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) ZoomedView.this.getLayoutParams();
> +            if ((touchState != MotionEvent.ACTION_MOVE) && (Math.abs((int) (event.getRawX() - originRawX)) < 1)
> +                    && (Math.abs((int) (event.getRawY() - originRawY)) < 1)) {


Instead of hard-coding 1 here I would suggest using PanZoomController.CLICK_THRESHOLD, or maybe PanZoomController.PAN_THRESHOLD. You can see how those are used and decide what's appropriate.

@@ +133,5 @@
> +
> +    public ZoomedView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        convertedPosition = new PointF();
> +        returnValue = new PointF();


Get rid of convertedPosition and use local variables instead. You can keep returnValue as-is to avoid allocating lots of new PointF instances. The way you have it set up now, convertedPosition is assigned a new PointF here which is never actually used. And then in all the functions where convertedPosition is used, it is modified to point to returnValue anyway.

@@ +254,5 @@
> +        leftMarginMin = (int) offset.x;
> +
> +        if (newTopMargin < topMarginMin) {
> +            newLayoutParams.topMargin = topMarginMin;
> +        } else if (newTopMargin + viewHeight >= parentHeight) {


Use > instead of >= for consistency

@@ +267,5 @@
> +
> +        setLayoutParams(newLayoutParams);
> +        convertedPosition = getUnzoomedPositionFromPointInZoomedView(0, 0);
> +        xLastPosition = Math.round(convertedPosition.x);
> +        yLastPosition = Math.round(convertedPosition.y);


store [xy]LastPosition as a Point instead of separate ints, and use org.mozilla.gecko.gfx.PointUtils.round on convertedPosition to compute it.

@@ +384,5 @@
> +                if (layerView == null) {
> +                    return;
> +                }
> +                shouldBlockUpdate(false);
> +                refreshZoomedViewSize(viewport);


Probably don't need this null check here since there's one inside refreshZoomedViewSize anyway.

@@ +482,5 @@
> +        final int xPos = (int) (origin.x - offset.x) + xLastPosition;
> +        final int yPos = (int) (origin.y - offset.y) + yLastPosition;
> +
> +        GeckoEvent e = GeckoEvent.createZoomedViewEvent(tabId, xPos, yPos, viewWidth,
> +                viewHeight, (float) (2.0 * metrics.zoomFactor), buffer);


Use ZOOM_FACTOR instead of 2.0

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +86,5 @@
>       *    that because mViewportMetrics might get reassigned in between reading the different
>       *    fields. */
>      private volatile ImmutableViewportMetrics mViewportMetrics;
> +    private LayerView.OnMetricsChangedListener mDynamicToolbarViewportChangeListener;
> +    private LayerView.OnMetricsChangedListener mZoomedViewViewportChangeListener;


See comment in BrowserApp, I would rather have a List<LayerView.OnMetricsChangedListener> here.

@@ +917,5 @@
> +        if (mDynamicToolbarViewportChangeListener != null) {
> +        	mDynamicToolbarViewportChangeListener.onPanZoomStopped();
> +        }
> +        if (mZoomedViewViewportChangeListener != null) {
> +        	mZoomedViewViewportChangeListener.onPanZoomStopped();


indentation

::: mobile/android/base/gfx/LayerRenderer.java
@@ +97,5 @@
>      private int mTMatrixHandle;
>  
> +    private List<LayerView.OnZoomedViewListener> mZoomedViewListeners;
> +    private float mViewLeft = 0.0f;
> +    private float mViewTop = 0.0f;


no need to initialize these, they are 0 by default. Also i would prefer if they were called mLastViewLeft and mLastViewTop

@@ +597,5 @@
>              runRenderTasks(mTasks, true, mFrameStartTime);
>  
>          }
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){


nit: space before {. Also make this private

@@ +600,5 @@
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){
> +            // Concurrently update of mZoomedViewListeners should not be an issue here
> +            if (mZoomedViewListeners.size() == 0) {
> +                return;


I think concurrent update could be an issue, since this code runs on the compositor thread and all the other accesses/mutations of mZoomedViewListeners are on the UI thread. You could probably get away with adding a volatile bool mHasZoomedViewListeners and then reading that here, setting/clearing it on the UI thread when listeners are added and removed. Because this function gets called a lot we want to avoid adding synchronization here so the volatile bool might be a cheap solution. Be sure to document the purpose of the bool if you add it.

::: mobile/android/base/gfx/LayerView.java
@@ +526,5 @@
>  
> +    @WrapElementForJNI(allowMultithread = true, stubName = "updateZoomedView")
> +    public static void updateZoomedView(ByteBuffer data) {
> +        data.position(0);
> +        LayerView layerView = GeckoAppShell.getLayerView();


I would add a comment here that this function runs on the Gecko main thread. Also I would move the data.position(0) call to be inside the LayerRenderer.updateZoomedView function, so that it happens just before handing the buffer to the listener. That way if one listener moves the position you'll still reset it before handing it to the next listener.

@@ +529,5 @@
> +        data.position(0);
> +        LayerView layerView = GeckoAppShell.getLayerView();
> +        if (layerView != null) {
> +            LayerRenderer layerRenderer = layerView.getRenderer();
> +            if (layerRenderer != null){


nit: space before {

@@ +533,5 @@
> +            if (layerRenderer != null){
> +                layerRenderer.updateZoomedView(data);
> +            }
> +        }
> +        return;


Unnecessary return

@@ +678,5 @@
> +    }
> +
> +    // Public hooks for zoomed view
> +
> +    public interface OnZoomedViewListener {


Would prefer to call this "ZoomedViewListener" (i.e. drop the "On")

::: widget/android/AndroidBridge.cpp
@@ +1705,5 @@
>  }
>  
> +static float
> +GetScaleFactor(nsPresContext* mPresContext) {
> +  nsIPresShell* presShell = mPresContext->PresShell();


The "m" prefix is used for class member variables. In this case it's not a member variable, so just use presContext or aPresContext.

@@ +1707,5 @@
> +static float
> +GetScaleFactor(nsPresContext* mPresContext) {
> +  nsIPresShell* presShell = mPresContext->PresShell();
> +  LayoutDeviceToLayerScale cumulativeResolution(presShell->GetCumulativeResolution().width);
> +  return cumulativeResolution.scale;


This file seems to have 4-space indentation in general, would be good to maintain that for consistency.

@@ +1711,5 @@
> +  return cumulativeResolution.scale;
> +}
> +
> +nsresult
> +AndroidBridge::CaptureZoomedView (nsIDOMWindow *window, nsIntRect zoomedViewRect, Object::Param buffer,


nit: no space before (

@@ +1718,5 @@
> +  struct timeval        timeEnd;
> +  struct timeval        timeEndAfter;
> +  struct timeval        timeStart;
> +  struct timeval        res;
> +  gettimeofday (&timeStart, NULL);


Looks like this stuff was for local testing/timing? Please remove it as it doesn't seem to be used any more.

@@ +1723,5 @@
> +
> +  if (!buffer)
> +    return NS_ERROR_FAILURE;
> +
> +  nsCOMPtr < nsIDOMWindowUtils > utils = do_GetInterface (window);


remove spaces around < and >, here and throughout this function. Also remove the space before (, throughout the function. See the rest of the code in this file for coding style, or refer to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +1750,5 @@
> +  nsCOMPtr < nsIPresShell > presShell = presContext->PresShell ();
> +
> +  float scaleFactor = GetScaleFactor(presContext) ;
> +
> +      nscolor bgColor = NS_RGB (255, 255, 255);


indentation
(Assignee)

Updated

3 years ago
Depends on: 663803
(Assignee)

Comment 1

3 years ago
Created attachment 8557594 [details] [diff] [review]
patch-01022015 1-Bug_1127909___Nits_in_ZoomedView_code_r_kats.patch

Kats, I made the code changes except the following one:

>>>>>
Rather than having OnMetricsChangedDynamicToolbarViewportListener and OnMetricsChangedZoomedViewportListener I would prefer you just had "addMetricsChangedListener" and "removeMetricsChangedListener" and store a List<LayerView.OnMetricsChangedListener> in GeckoLayerClient.
<<<<<

Your solution was my first choice but read the comment 116 in bug 663803 [1]; it explains the reason why we didn’t implement it like that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c116
Attachment #8557594 - Flags: review?(bugmail.mozilla)
Comment on attachment 8557594 [details] [diff] [review]
patch-01022015 1-Bug_1127909___Nits_in_ZoomedView_code_r_kats.patch

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

Thanks!
Attachment #8557594 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

3 years ago
Kats, one test failed (rc1 for Android 2.3) on the try server. What should we do in this case?
Flags: needinfo?(bugmail.mozilla)
I'm pretty sure that failure is an known intermittent failure. I did a couple of retriggers to double-check; if those are green then we should be fine to land the patch.
Flags: needinfo?(bugmail.mozilla)
The retriggers were green as I expected, so I landed the patch on fx-team for you:

https://hg.mozilla.org/integration/fx-team/rev/b55925d41108
https://hg.mozilla.org/mozilla-central/rev/b55925d41108
Assignee: nobody → domivinc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.