Closed Bug 994724 Opened 6 years ago Closed 5 years ago

Clean up ZoomHelper logic

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: Margaret, Assigned: rricard, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

This is a follow-up to bug 990259. The patch that landed in that bug fixed a regression, but there's more clean-up that could be done and ride the trains.

Robin, I'm assigning this to you, since you were already working on it, but this is less time-sensitive than bug 990259 :)
Ok, I still have to fix my broken test chain but I intend to take care of it anyway ...
Ok, so I just redone the same patch as before but I'm still unable to run tests. I'll just try to catch support on  irc ...
Ok, I just got back my testing toolchain. That's stupid I didn't figure out what's wrong before ... Time to solve this issue anyway !
Blocks: 1008328
Ok, I have the patch that failed on tbpl on my computer, but I can't reproduce the bug on my Nexus 5 or on an emulator ... So I will merge with the last master branch and send you my current patch ...
Attached patch 994724.patch (obsolete) — Splinter Review
Shouldn't pass on tbpl while I'm still retrying to get the same error locally but it's worth a try I think ...
Attachment #8420569 - Flags: review?(margaret.leibovic)
(In reply to Robin Ricard from comment #5)
> Created attachment 8420569 [details] [diff] [review]
> 994724.patch
> 
> Shouldn't pass on tbpl while I'm still retrying to get the same error
> locally but it's worth a try I think ...

Sorry I've been so slow to respond here! I'm going to take a look at this patch now.

In the meantime, you should file a bug to get level 1 commit access so that you can push your own patches to try server :) Directions are here: http://www.mozilla.org/hacking/commit-access-policy/

You should cc me to the bug and I'll vouch for you!
Ok, I'll do that, thank you !
Comment on attachment 8420569 [details] [diff] [review]
994724.patch

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

Could you update your diff configuration to provider 8 lines of context? It's hard to read patches without much context.

I'd also like kats to take a look at this to help verify the logic of what's going on. I feel like the logic has moved around through various refactoring patches, and maybe it's time to reconsider how we're doing things here.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +85,5 @@
>      const margin = 15;
>  
> +    if(!rect.h || !rect.w) {
> +      rect.h = rect.height;
> +      rect.w = rect.width;

Looking at the implementation of ElementTouchHelper.getBoundingContentRect, there will never be a rect.height/rect.width, so this logic is incorrect. However, it looks like there will always be a rect.h and rect.w, so we could just get rid of this if statement.

@@ +90,4 @@
>      }
>  
>      let viewport = BrowserApp.selectedTab.getViewport();
> +    let bRect = new Rect(aCanScrollHorizontally ? Math.max(viewport.cssPageLeft, rect.x - margin) : viewport.cssX,

While we're in here, can we give this a more descriptive name than bRect? We should also add some comments about the goal of these computations.

@@ +126,2 @@
>  
>      let rect = {};

Can we refactor this to actually use a Rect object instead of this makeshift object? Then create the object we're going to send to Java down below?
Attachment #8420569 - Flags: feedback?(bugmail.mozilla)
Okay I'll see that when I get more time, I'm kinda in a rush right now with my finals approaching. But it shouldn't be excluded that I'll start to work on this in the few next days anyway !
Comment on attachment 8420569 [details] [diff] [review]
994724.patch

Clearing review for now until we get a try push.
Attachment #8420569 - Flags: review?(margaret.leibovic)
Comment on attachment 8420569 [details] [diff] [review]
994724.patch

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

I'm not really sure this patch provides much benefit. The code is marginally cleaner but it's also functionally different now. If I'm reading the code correctly, calling zoomToRect no longer normalizes the rect to ensure it is valid and within the screen bounds, so this might have unexpected consequences.
Attachment #8420569 - Flags: feedback?(bugmail.mozilla) → feedback-
Attached patch Clean up ZoomHelper logic (obsolete) — Splinter Review
Here is what I intend to push to try as soon as I get access to it.
Depends on: 1014113
No longer depends on: 1014113
Blocks: 1014113
Mentor: margaret.leibovic
Hey Robin, how's it going? Did you ever get try server access?
Flags: needinfo?(ricard.robin)
It's not going that well actually ! I don't have a lot of time to work on that and it's my first problem. But that doesn't keep me from trying.

My second problem is that my local test environment is broken again and I don't see why (it worked before, now it just fails). If only it worked, I think I would have already fixed it but I just can't test it.

My third problem is that I primarily use git for work and I just didn't found out how to send try pushes with the moz git tools (I miss some parameters I think).

Anyway, I think I'll need a good old computer reset before working on it again (There's something wrong with my adk, but I can't see what... and even if I reinstall it, nothing changes ...).

So, I hope I'll get that fixed soon but right now, I just can't reset my laptop because I use it every day for work ...
Flags: needinfo?(ricard.robin)
If you need it soon, I don't think I'll be able to deliver.

But if I can ship it in about 1 month, I think can sort out my time/build/push problems and deliver a patch.
Well, cool ! I took some time to fix my Android SDK installation and learn to launch a try build. From now on, I'm back to operational. So I think I'll be able to finally kick this bug out in the next few days !
So, I have a patch ready but it does not pass sometimes on tbpl due to known intermittent bugs.
Attached patch Clean up ZoomHelper logic (obsolete) — Splinter Review
Here's the try build on this patch: https://tbpl.mozilla.org/?tree=Try&rev=9f7a16acbb6b
Attachment #8488145 - Flags: review?(margaret.leibovic)
Attachment #8426332 - Attachment is obsolete: true
Attachment #8420569 - Attachment is obsolete: true
Comment on attachment 8488145 [details] [diff] [review]
Clean up ZoomHelper logic

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

Thanks for the try run! Looks good, just two small comments to address. If you upload a new version that addresses these, I can land this patch for you.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +87,3 @@
>      let viewport = BrowserApp.selectedTab.getViewport();
> +    rect = new Rect(aCanScrollHorizontally ? Math.max(viewport.cssPageLeft, rect.x - margin) : viewport.cssX,
> +                         rect.y,

Nit: Align the indentation here with the first parameter in the line above (similar to how it was before).

@@ +119,2 @@
>  
> +  zoomToRect: function(aRect, aClickY = -1) {

Add a comment above this method explaining what it does.
Attachment #8488145 - Flags: review?(margaret.leibovic) → review+
Attachment #8490629 - Attachment description: Clean up ZoomHelper logic → Clean up ZoomHelper logic I did the refinements but didn't launch any try for just comments and indentation changes !
Attachment #8490629 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79bf6ebacb09
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Depends on: 1070072
This seems to have regressed the ability to double-tap to zoom; see my filed bug above.
I backed this out:
https://hg.mozilla.org/integration/fx-team/rev/43bc9236ea46
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8490629 [details] [diff] [review]
Clean up ZoomHelper logic

I did the refinements but didn't launch any try for just comments and indentation changes !

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

::: mobile/android/chrome/content/ZoomHelper.js
@@ +139,5 @@
>          rect.y = cssTapY - (rect.h / 2);
>        }
>      }
>  
>      if (rect.w > viewport.cssWidth || rect.h > viewport.cssHeight) {

Looks like I failed to catch this problem.

Because rect is now a Rect, it doesn't have `w` and `h` properties. The simplest fix here would be to avoid refactoring rect to be a Rect, and keep it as an object.
Attachment #8490629 - Flags: review+ → review-
Or I could just refactor these… I don't know, your call. I liked the idea to have a proper Rect object.
Flags: needinfo?(margaret.leibovic)
Maybe it's another refactor to do, but it could be interesting normalize all of this (settle on something between w and h or width and height throughout the whole project), but that's maybe not possible…
It's fine to use the Rect object and update where the variable is used. Let's just make sure we update it properly! In general, it's best to keep changes as minimal as possible, but since this is already a refactor, it is okay to err on the side of cleaning things up.

Also, something interesting, it appears that the original landing of this patch caused a noticeable improvement in our checkerboarding performance metrics:
http://graphs.mozilla.org/graph.html#tests=%5B%5B201,64,29%5D%5D&sel=none&displayrange=7&datatype=running

I noticed this because backing it out caused a regression :)

So I would be eager to fix this patch up and re-land it, to see if that performance improvement stays!
Flags: needinfo?(margaret.leibovic)
Ok, that's fine for me, I'm going to take care of it tonight!
Attached patch Fix rect object issues. (obsolete) — Splinter Review
Try build: https://tbpl.mozilla.org/?tree=Try&rev=bdcbce04d96d
Attachment #8490629 - Attachment is obsolete: true
Attachment #8493304 - Flags: review?(margaret.leibovic)
Comment on attachment 8493304 [details] [diff] [review]
Fix rect object issues.

I just applied the patch, and followed Aaron's STR from bug 1070072 and ran into this error:
E/GeckoConsole(16885): [JavaScript Error: "ReferenceError: viewport is not defined" {file: "chrome://browser/content/ZoomHelper.js" line: 129}]
Attachment #8493304 - Flags: review?(margaret.leibovic) → review-
Ok, that's new, I have to sync my repo I think…
Attached patch Clean up ZoomHelper logic (obsolete) — Splinter Review
Ok, this one was easy, I should have done it days ago ...

This time I think we have something working ! I relaunch a try and we'll see.
Sorry for the delay !

Anyway, here's the try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=737b2feb52c1
Attachment #8501713 - Flags: review?(margaret.leibovic)
Attachment #8493304 - Attachment is obsolete: true
Comment on attachment 8501713 [details] [diff] [review]
Clean up ZoomHelper logic

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

Looking good. Let's just update the zoomToRect call in aboutReader.js, then we can land this.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +120,5 @@
> +  /* Zoom to a specific part of the screen defined by a rect,
> +   * optionally keeping a particular part of it in view
> +   * if it is really tall.
> +   */
> +  zoomToRect: function(aRect, aClickY = -1) {

It looks like about:reader uses zoomToRect with more parameters:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#298

Since those are both false, I think we can just transition this call to zoomToRect(newRect), but we'll need to make sure that still works as intended.

@@ +133,2 @@
>  
>      rect.type = "Browser:ZoomToRect";

You can also just declare type as a property in the object above.

It would be nice to rename that from `rect` to `msg`, since it's actually a message object that we pass to Java at the end of this function call, but we could file a separate [good first bug] about that.
Attachment #8501713 - Flags: review?(margaret.leibovic) → feedback+
Do whatever you think it's the best about filing a new bug. I could fix it but it's up to you !
So I tried it without the false arguments, it seems to work as usual but I don't know if a test covers that. I'll start a try and check for failures but that's not really likely, it should have failed before if there was a problem on this part.
Attachment #8501713 - Attachment is obsolete: true
Attachment #8504532 - Flags: review?(margaret.leibovic)
Attachment #8504536 - Attachment description: Clean up ZoomHelper logic → Clean up ZoomHelper logic That's the same patch with rect renamed to msg (merge the one you prefer)
Comment on attachment 8504536 [details] [diff] [review]
Clean up ZoomHelper logic


That's the same patch with rect renamed to msg (merge the one you prefer)

Let's keep the original scope of this bug.
Attachment #8504536 - Attachment is obsolete: true
Attachment #8504536 - Flags: review?(margaret.leibovic)
Comment on attachment 8504532 [details] [diff] [review]
Clean up ZoomHelper logic

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

Looks good to me.

::: mobile/android/chrome/content/aboutReader.js
@@ +294,5 @@
>  
>      this._setToolbarVisibility(false);
>      this._setBrowserToolbarVisiblity(false);
>      this._scrolled  = true;
> +    ZoomHelper.zoomToRect(newRect, -1);

Technically we don't need to pass -1 here either, since it's the default.
Attachment #8504532 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39cc3da6233e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.