1504362 - Consider unifying nsViewportInfo::mDefaultZoom and mDefaultZoomvalid into Maybe<CSSToScreenScale>

Status: RESOLVED WONTFIX

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

Comment 1

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

I don't know if this is doable. We set mDefaultZoom to a clamped value and use it in places even when mDefaultZoomValid is false. So if we replace that state with a Nothing() we lose information that we currently have. It would require some change in semantics on the calling code side to make this possible. Hiro, is that what you had in mind?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Hmm, actually I hadn't checked all call sites of nsViewportInfo::GetDefaultZoom() at that time.

Here are the call sites where the function gets called without checking the result of nsViewportInfo::IsDefaultZoomValue():

• nsDOMWindowUtils::GetViewportInfo
  The only place we use the function is in devtools, but the default zoom value is not used there at all. (Though actually there is another place calling GetViewportInfo in test utils, it doesn't matter I suppose)
• nsIPresShell::DetermineFontSizeInflationState
  If we defer doing the clamp which is now done in nsViewportInfo::ConstrainViewportValue() in here, it will work as expected (just like as you mentioned)
• ComputeZoomConstraintsFromViewportInfo
  This looks the same situation above, we can defer the clamp in here as well.

That's being said, it seems that it's probably not worth doing make mDefaultZoom and mDefaultZoomValid into Maybe<CSSToScreenScale> for now.

I am going to close this bug as WONTFIX, please feel free to reopen this bug if someone wants to fix this.

Thanks!