Closed Bug 1197824 Opened 4 years ago Closed 4 years ago

Update Fennec code to use ZoomConstraintsClient instead of the big pile of browser.js code

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(9 files, 1 obsolete file)

1.82 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.47 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.56 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.29 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.25 KB, patch
snorp
: review+
Details | Diff | Splinter Review
9.42 KB, patch
snorp
: review+
Details | Diff | Splinter Review
8.72 KB, patch
kats
: review+
Details | Diff | Splinter Review
10.83 KB, patch
snorp
: review+
Details | Diff | Splinter Review
10.94 KB, patch
botond
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8655056 - Flags: review?(snorp) → review+
Attachment #8655066 - Flags: review?(snorp) → review+
Comment on attachment 8655068 [details] [diff] [review]
Part 7 - Update browser.js to use the new message and throw out unneeded code

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

This is lovely.
Attachment #8655068 - Flags: review?(snorp) → review+
I did a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ab9d0715a1&group_state=expanded which showed R6 failures on Android. I'm bisecting the queue to figure out where it's coming from (seems to be in the first half somewhere, possibly from the patches for bug 1200303 or bug 1200402).
Updated try push at [1] is green. There was an error in part 1 which resulted in the JavaPanZoomController using maxZoom = 4.0 instead of maxZoom = 0. Correcting that (using the incremental change at [2] fixed the problem.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb1275e8e541
[2] https://hg.mozilla.org/try/rev/806ca7d0f012
Updated part 1 patch as described above; carrying r+
Attachment #8655056 - Attachment is obsolete: true
Attachment #8655982 - Flags: review+
There's more code deletion that can be done from browser.js. Still testing this locally to make sure I didn't screw it up.
Attachment #8655984 - Attachment description: Part 8 - More code deletion from browser.js (WIP) → Part 8 - More code deletion from browser.js
Attachment #8655984 - Flags: review?(snorp)
Attachment #8655058 - Flags: review?(botond) → review+
Comment on attachment 8655059 [details] [diff] [review]
Part 3 - Some cleanup in GetViewportInfo

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

If we're going to be maintaining this invariant ("the default value of allowDoubleTapToZoom is the same as the initial value of allowZoom"), then, given that no one calls nsViewportInfo::SetAllowDoubleTapZoom(), nor does anyone change nsViewportInfo::mAllowZoom, is there any value in storing nsViewportInfo::mAllowDoubleTapZoom in the first place?
Attachment #8655059 - Flags: review?(botond) → review+
Attachment #8655061 - Flags: review?(botond) → review+
Attachment #8655065 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #13)
> If we're going to be maintaining this invariant ("the default value of
> allowDoubleTapToZoom is the same as the initial value of allowZoom"), then,
> given that no one calls nsViewportInfo::SetAllowDoubleTapZoom(), nor does
> anyone change nsViewportInfo::mAllowZoom, is there any value in storing
> nsViewportInfo::mAllowDoubleTapZoom in the first place?

Yeah that's a excellent point, I can get rid of it.
Attachment #8656123 - Flags: review?(botond) → review+
Attachment #8655984 - Flags: review?(snorp) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.