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.

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?

(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.

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.

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.

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.

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. :> )

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. ;)

Thanks for the feedback! Here is a new version of the patch with an updated commit message.

Submitting the correct patch this time...

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!

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!

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!

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.

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/