802640 - Refactor some code in browser.js

Status: RESOLVED FIXED

Description

[Kartikaya Gupta (email:[e-mail])]

I have these two refactoring patches in my queue for some of the readability prototypes I was/am working on, and I would really like to land them to avoid having to keep rebasing and manually merging them.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Extract a zoomToElement function

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch 1 with whitespace changes ignored

To more easily see the diff

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Extract a method to handle touchstart

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 672325 [details] [diff] [review]
(1/2) Extract a zoomToElement function

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

::: mobile/android/chrome/content/browser.js
@@ +3773,2 @@
>  
> +  _zoomToElement: function(aElement, aClickY) {

aClickY should probably be optional. You can use the new defaultParameters stuff to make it = -1 (which we can use to force zooming to really tall elements?):

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/default_parameters

@@ +3799,5 @@
> +    // if the block we're zooming to is really tall, and the user double-tapped
> +    // more than a screenful of height from the top of it, then adjust the y-coordinate
> +    // so that we center the actual point the user double-tapped upon. this prevents
> +    // flying to the top of a page when double-tapping to zoom in (bug 761721).
> +    // the 1.2 multiplier is just a little fuzz to compensate for bRect including horizontal

NIT: Update these comments to not talk about double tap and just talk about elements that are really tall.

@@ +3801,5 @@
> +    // so that we center the actual point the user double-tapped upon. this prevents
> +    // flying to the top of a page when double-tapping to zoom in (bug 761721).
> +    // the 1.2 multiplier is just a little fuzz to compensate for bRect including horizontal
> +    // margins but not vertical ones.
> +    let cssTapY = viewport.cssY + aClickY;

I think we probably want a way to skip this. I.e. wrap in a if (aClickY >= 0)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Updated as per review comments and landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8459554cbfe9
https://hg.mozilla.org/integration/mozilla-inbound/rev/02bcbfb28383

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8459554cbfe9
https://hg.mozilla.org/mozilla-central/rev/02bcbfb28383