reflozPinchSeen is unused in browser.js

RESOLVED FIXED in Firefox 29

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwir3, Assigned: karinsofia)

Tracking

unspecified
Firefox 29
All
Android

Details

(Whiteboard: [mentor=jwir3][lang=javascript])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
In the following area of browser.js for android:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3127

It appears that the variable 'relozPinchSeen' is unused (that is, it's never set to true anywhere in the browser.js). However, simple removal of this variable (and assumption that it's always false) causes reflow-on-zoom to break. We should look into this further and remove this extraneous variable.
(Reporter)

Updated

5 years ago
OS: Linux → Android
Hardware: x86_64 → All
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jwir3][lang=javascript]
(Assignee)

Comment 1

5 years ago
When removing the variable I experience the same result as when building the current code: 
1. You can zoom in when the page is loading.
2. Zoom out (to the default size) is possible even after the page finish to load. 
(Is this the expected behavior?) 

Could you be more descriptive about what functionality that breaks?
Flags: needinfo?(jaywir3)
(Reporter)

Comment 2

5 years ago
(In reply to Sofia Larsson from comment #1)
> When removing the variable I experience the same result as when building the
> current code: 
> 1. You can zoom in when the page is loading.
> 2. Zoom out (to the default size) is possible even after the page finish to
> load. 
> (Is this the expected behavior?) 
> 
> Could you be more descriptive about what functionality that breaks?

Hi Sofia:

So, when I was building locally, I noticed that double-tap to reflow (which I believe is no longer in nightly, so you'll probably have to set the pref 'browser.zoom.reflowOnZoom' in about:config to true) was broken when I tried to use it without the variable.

Mainly, what I would expect to happen is that the functionality AFTER the variable is removed from browser.js to be the same (with respect to reflow-on-zoom) before the variable is removed. That is, if the pref mentioned above is enabled, then double-tapping will generate a reflow IF the font size is increased. Since it is based off of the font inflation preferences, you'll likely need to make these larger as well. Try setting font.size.inflation.minTwips to 800, then take a look at a page that has -moz-text-size-adjust: none set on the body text (effectively suppressing font inflation). Double tap on the text, and then see if it a) zooms in, and b) reflows the text so that lines wrap on screen width.
Flags: needinfo?(jaywir3)
(Assignee)

Comment 3

5 years ago
Created attachment 8341220 [details] [diff] [review]
Remove unused 'logic' for controlling the reflow

This part is never run due to the variable reflozPinchSeen, as mentioned earlier, is always false.
The attempted logic for zooming out, resetting the max box line, is already handled in _zoomOut.
Attachment #8341220 - Flags: review?(jaywir3)
(Reporter)

Comment 4

5 years ago
Comment on attachment 8341220 [details] [diff] [review]
Remove unused 'logic' for controlling the reflow

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

::: mobile/android/chrome/content/browser.js
@@ -3164,5 @@
> -      // In this case, the user pinch-zoomed in, so we don't want to
> -      // preserve position as we would with reflow-on-zoom.
> -      BrowserApp.selectedTab.probablyNeedRefloz = false;
> -      BrowserApp.selectedTab._mReflozPoint = null;
> -    }

This logic isn't quite right... I think that if we remove the reflozPinchSeen variable, then the remaining logic should still stay in place. In other words, I think it should look something more like this:

if (isZooming && aViewport.zoom < 1.0) {		
// In this case, we want to restore the max line box width,		
// because we are pinch-zooming to zoom out.		
BrowserEventHandler.resetMaxLineBoxWidth();		
} else if (isZooming) {		
// In this case, the user pinch-zoomed in, so we don't want to		
// preserve position as we would with reflow-on-zoom.		
BrowserApp.selectedTab.probablyNeedRefloz = false;		
BrowserApp.selectedTab._mReflozPoint = null;		
}

Also, have you verified that the reflow-on-zoom behavior is the same even when this variable is removed? I vaguely remember reflow on zoom not functioning correctly the last time I tried to remove this variable, but things could certainly have changed since then.
Attachment #8341220 - Flags: review?(jaywir3) → review-
(Assignee)

Comment 5

5 years ago
Hi!

Thank you for reviewing. I realize I didn't explain my solution properly, so I'll give it another try.
Since BrowserApp.selectedTab.reflozPinchSeen is never set to true will this part of the code never run. Removing it will therefore not affect the current behavior of the application at all.
 
Simply removing the variable will cause the reflow-on-zoom to break, as you mention, since the code will suddenly run and appears not to be fully correct.

This part of the code seems like something which has been accidentally forgotten when refactoring the code and is now handled by other parts of the code:

if (isZooming && aViewport.zoom < 1.0) {		
// In this case, we want to restore the max line box width,		
// because we are pinch-zooming to zoom out.		
BrowserEventHandler.resetMaxLineBoxWidth();

The case above is already handled by _zoomOut, which gets triggered when a double tap has been performed and no element in that area has been found.
		
} else if (isZooming) {		
// In this case, the user pinch-zoomed in, so we don't want to		
// preserve position as we would with reflow-on-zoom.		
BrowserApp.selectedTab.probablyNeedRefloz = false;		
BrowserApp.selectedTab._mReflozPoint = null;		
}

This seems not to be fully correct but as well handled as the BrowserApp.selectedTab.probablyNeedRefloz is set to true when a double tap has been performed and when reflow-on-zoom is enabled in onDoubleTap. As soon as the box width has changed it's again set to false.

I hope I explained my self a little better this time. :) Please tell me if any other than the current behavior is expected.
(Reporter)

Comment 6

5 years ago
Comment on attachment 8341220 [details] [diff] [review]
Remove unused 'logic' for controlling the reflow

(In reply to Sofia Larsson (:karinsofia) from comment #5)
> Hi!
> 
> Thank you for reviewing. I realize I didn't explain my solution properly, so
> I'll give it another try.
> Since BrowserApp.selectedTab.reflozPinchSeen is never set to true will this
> part of the code never run. Removing it will therefore not affect the
> current behavior of the application at all.
>  
> Simply removing the variable will cause the reflow-on-zoom to break, as you
> mention, since the code will suddenly run and appears not to be fully
> correct.
> 
> This part of the code seems like something which has been accidentally
> forgotten when refactoring the code and is now handled by other parts of the
> code:
> 
> if (isZooming && aViewport.zoom < 1.0) {		
> // In this case, we want to restore the max line box width,		
> // because we are pinch-zooming to zoom out.		
> BrowserEventHandler.resetMaxLineBoxWidth();
> 
> The case above is already handled by _zoomOut, which gets triggered when a
> double tap has been performed and no element in that area has been found.
> 		
> } else if (isZooming) {		
> // In this case, the user pinch-zoomed in, so we don't want to		
> // preserve position as we would with reflow-on-zoom.		
> BrowserApp.selectedTab.probablyNeedRefloz = false;		
> BrowserApp.selectedTab._mReflozPoint = null;		
> }
> 
> This seems not to be fully correct but as well handled as the
> BrowserApp.selectedTab.probablyNeedRefloz is set to true when a double tap
> has been performed and when reflow-on-zoom is enabled in onDoubleTap. As
> soon as the box width has changed it's again set to false.
> 
> I hope I explained my self a little better this time. :) Please tell me if
> any other than the current behavior is expected.

Hi Sofia:

Sorry this has taken me so long to get back to you. I see where you're coming from with this, and I think you're right, after looking through this - the logic here is incorrect.  However, I think you should have kats take a look at this as well.

So, r=jwir3 assuming you have kats just briefly look it over to make sure it meets his expectations (I know he's working on things related to this, so I don't want to r+ something that steps all over his code. :> )
Attachment #8341220 - Flags: review- → review+
Flags: needinfo?(bugmail.mozilla)
(Reporter)

Comment 7

5 years ago
One last thing -

You might want to change the commit message slightly. It's unnecessary to have [PATCH] in the commit message, and the commit message should look something like this:

Bug # - <description of change and reason why change is necessary> [r=<reviewer(s)]

So, I'd change it to look like:

Bug 922682 - Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js [r=jwir3,kats]

That's assuming kats says it's good and r+'s it. ;)
(Assignee)

Comment 8

5 years ago
Created attachment 8351653 [details] [diff] [review]
Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js

Thanks for the feedback! Here is a new version of the patch with an updated commit message.
Attachment #8341220 - Attachment is obsolete: true
Attachment #8351653 - Flags: review?(jaywir3)
Attachment #8351653 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 9

5 years ago
Created attachment 8351654 [details] [diff] [review]
Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js

Submitting the correct patch this time...
Attachment #8351653 - Attachment is obsolete: true
Attachment #8351653 - Flags: review?(jaywir3)
Attachment #8351653 - Flags: review?(bugmail.mozilla)
Attachment #8351654 - Flags: review?(wjohnston)
Attachment #8351654 - Flags: review?(bugmail.mozilla)
Comment on attachment 8351654 [details] [diff] [review]
Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js

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

Sorry for the delayed review; I was away over the holiday break. This patch looks fine to me - since reflozPinchSeen is never set to true it makes sense that this is all dead code and can be axed. Thanks for going through the full code path and ensuring the rest of the logic still makes sense!
Attachment #8351654 - Flags: review?(bugmail.mozilla) → review+
Flags: needinfo?(bugmail.mozilla)
I also just noticed that the patch has unfortunately bitrotted a little bit and doesn't apply cleanly. I can rebase the patch and check it in if you want; if not, you can rebase the patch, upload the new version, and set the "checkin-needed" keyword on this bug so the sheriffs can land it. If you rebase it yourself, please also remove the diffstat from the commit message (the stuff below the bug number/commit message). Thanks!
(Assignee)

Comment 12

5 years ago
Created attachment 8355902 [details] [diff] [review]
Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js
Attachment #8351654 - Attachment is obsolete: true
Attachment #8351654 - Flags: review?(wjohnston)
(Assignee)

Comment 13

5 years ago
Sorry for the delay!

Notable is that a new method call, for clearing pending reflow actions, has been added to the if clause. But the patch won't, as earlier affect this at all, since this can never be performed. But maybe this functionality should be looked over?

I'm afraid I don't have the access (?) or don't find the option to set the "checkin-needed" keyword.
Could you help me with this?
Thanks!
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8355902 [details] [diff] [review]
Remove unused 'logic' for setting the viewport to simplify reflow-on-zoom code in browser.js

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

Yeah I noticed the new call but I think it is still dead code and can be removed safely. So this patch looks good. I will set the checkin-needed flag on the bug; I didn't realize it was restricted.
Attachment #8355902 - Flags: review+
Thanks for putting together this patch, by the way! If you're interested in taking on some more bugs you can find some good ones using the search tool at http://www.joshmatthews.net/bugsahoy/
Flags: needinfo?(bugmail.mozilla)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d73cb462dbed
Assignee: nobody → karinsofiapaulina
Keywords: checkin-needed
Whiteboard: [mentor=jwir3][lang=javascript] → [mentor=jwir3][lang=javascript][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d73cb462dbed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=jwir3][lang=javascript][fixed-in-fx-team] → [mentor=jwir3][lang=javascript]
Target Milestone: --- → Firefox 29
Product: Firefox for Android → Fennec Graveyard
You need to log in before you can comment on or make changes to this bug.