Closed
Bug 994724
Opened 10 years ago
Closed 10 years ago
Clean up ZoomHelper logic
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: Margaret, Assigned: rricard, Mentored)
References
Details
Attachments
(1 file, 7 obsolete files)
6.22 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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 :)
Assignee | ||
Comment 1•10 years ago
|
||
Ok, I still have to fix my broken test chain but I intend to take care of it anyway ...
Assignee | ||
Comment 2•10 years ago
|
||
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 ...
Assignee | ||
Comment 3•10 years ago
|
||
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 !
Assignee | ||
Comment 4•10 years ago
|
||
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 ...
Assignee | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
(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!
Assignee | ||
Comment 7•10 years ago
|
||
Ok, I'll do that, thank you !
Reporter | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 !
Reporter | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
Here is what I intend to push to try as soon as I get access to it.
Updated•10 years ago
|
Mentor: margaret.leibovic
Reporter | ||
Comment 13•10 years ago
|
||
Hey Robin, how's it going? Did you ever get try server access?
Flags: needinfo?(ricard.robin)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
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 !
Assignee | ||
Comment 17•10 years ago
|
||
So, I have a patch ready but it does not pass sometimes on tbpl due to known intermittent bugs.
Assignee | ||
Comment 18•10 years ago
|
||
Here's the try build on this patch: https://tbpl.mozilla.org/?tree=Try&rev=9f7a16acbb6b
Attachment #8488145 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•10 years ago
|
Attachment #8426332 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8420569 -
Attachment is obsolete: true
Reporter | ||
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8488145 -
Attachment is obsolete: true
Attachment #8490629 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•10 years ago
|
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 !
Reporter | ||
Updated•10 years ago
|
Attachment #8490629 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/79bf6ebacb09
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/79bf6ebacb09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Comment 23•10 years ago
|
||
This seems to have regressed the ability to double-tap to zoom; see my filed bug above.
Reporter | ||
Comment 24•10 years ago
|
||
I backed this out: https://hg.mozilla.org/integration/fx-team/rev/43bc9236ea46
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 25•10 years ago
|
||
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-
Assignee | ||
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
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…
Reporter | ||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
Ok, that's fine for me, I'm going to take care of it tonight!
Assignee | ||
Comment 30•10 years ago
|
||
Try build: https://tbpl.mozilla.org/?tree=Try&rev=bdcbce04d96d
Attachment #8490629 -
Attachment is obsolete: true
Attachment #8493304 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 31•10 years ago
|
||
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-
Assignee | ||
Comment 32•10 years ago
|
||
Ok, that's new, I have to sync my repo I think…
Assignee | ||
Comment 33•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8493304 -
Attachment is obsolete: true
Reporter | ||
Comment 34•10 years ago
|
||
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+
Assignee | ||
Comment 35•10 years ago
|
||
Do whatever you think it's the best about filing a new bug. I could fix it but it's up to you !
Assignee | ||
Comment 36•10 years ago
|
||
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.
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8501713 -
Attachment is obsolete: true
Attachment #8504532 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8504536 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 39•10 years ago
|
||
And the try push… : https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c400d84f8101
Reporter | ||
Comment 40•10 years ago
|
||
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)
Reporter | ||
Comment 41•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 42•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/39cc3da6233e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 43•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39cc3da6233e
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•