Closed Bug 1648193 Opened 3 months ago Closed 3 months ago

Make MobileViewportManager::UpdateResolution easier to understand

Categories

(Core :: Panning and Zooming, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

The UpdateResolution method has evolved over time, and now is actually called in 3 or 4 different scenarios with parameters that aren't really comparable from one scenario to another. The only thing they have in common is a few lines at the top and bottom of the function, but the bulk of the function is a big pile of nested if conditions that only gets invoked in one of the scenarios. This is not obvious when you first read the function and trying to understand it all is unnecessarily hard. I think it makes more sense to split this function into three different ones for the three main scenarios that it gets called for. This might be slightly less efficient code-wise but makes things a lot easier to understand.

This removes the two parameters to ShrinkToDisplaySizeIfNeeded and corresponding
parameters passed to UpdateResolution. The call site in nsGfxScrollFrame
gets the arguments from the MVM itself, so it seems silly to be getting things
from the MVM just to pass it back in. The other call sites are already in theMVM.
This change might be slightly less efficient because it re-computes the viewport
info when the caller might already have it but this isn't a hot code path so
I'm not too concerned.

This is a hard diff to read but fundamentally a pretty simple patch. The old
UpdateResolution function had a giant if condition in the middle conditioned
on the update type. Inside the ViewportSize branch there was a further nested
if conditioned on mIsFirstPaint. The function got split into three, with each
new function holding one of the three main blocks of code, along with a copy
of the stuff before and after the if condition. And then I simplified each
function individually to remove unnecessary variables, add some early-exits
and reduce nesting levels, etc.

Depends on D80937

Instead of having callers compute the cssToDev and pass it to the zoom/
resolution conversion functions, we can just do it in those functions directly.

Depends on D80938

Thanks for doing this cleanup :)

Severity: -- → S3
Priority: -- → P3
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ecc93a6635fc
Stop passing things to MVM that it already knows how to compute. r=hiro
https://hg.mozilla.org/integration/autoland/rev/f47b6c791922
Split UpdateResolution into three different functions. r=hiro
https://hg.mozilla.org/integration/autoland/rev/8ed7030eea26
Additional minor cleanup. r=hiro
You need to log in before you can comment on or make changes to this bug.