Closed Bug 910322 Opened 7 years ago Closed 7 years ago

nsViewportInfo should use types with units

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

nsViewportInfo currently uses a lot of ints and floats even though it's in the thick of things with respect to dealing with different coordinate systems. We should strongly type this code.
Version: 23 Branch → Trunk
Comment on attachment 796755 [details] [diff] [review]
Part 1 - Strongly type the zoom factors

Try run seems green enough
Attachment #796755 - Flags: review?(mbrubeck)
Attachment #796755 - Flags: review?(Ms2ger)
Attachment #796757 - Flags: review?(mbrubeck)
Attachment #796757 - Flags: review?(Ms2ger)
Attachment #796757 - Flags: review?(mbrubeck) → review+
Attachment #796755 - Flags: review?(mbrubeck) → review+
Do you still need my review? I'll try to look tomorrow if so.
Flags: needinfo?(bugmail.mozilla)
Yes please. In particular if you have any suggestions on refactoring/code cleanliness that would be useful.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 796755 [details] [diff] [review]
Part 1 - Strongly type the zoom factors

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

Just nits. r=me

::: content/base/public/nsViewportInfo.h
@@ +33,5 @@
>        mHeight(aDisplayHeight),
>        mAutoSize(true),
>        mAllowZoom(true)
>      {
> +        mMinZoom = kViewportMinScale * mozilla::CSSToLayoutDeviceScale(1.0);

So this is Scale<B, C> * Scale<A, B>? I think Scale<A, B> * Scale<B, C> tends to be more obviously correct, and I'd prefer removing the other operator.

::: content/base/src/nsDocument.cpp
@@ +6854,5 @@
>      if (NS_FAILED(errorCode)) {
>        mScaleMinFloat = kViewportMinScale;
>      }
>  
> +    mScaleMinFloat = (mScaleMinFloat < kViewportMaxScale) ? mScaleMinFloat : kViewportMaxScale;

I don't think we usually overparenthesize this, and the line is overlong. So:

    mScaleMinFloat = mScaleMinFloat < kViewportMaxScale
                     ? mScaleMinFloat
                     : kViewportMaxScale;

@@ +6855,5 @@
>        mScaleMinFloat = kViewportMinScale;
>      }
>  
> +    mScaleMinFloat = (mScaleMinFloat < kViewportMaxScale) ? mScaleMinFloat : kViewportMaxScale;
> +    mScaleMinFloat = (mScaleMinFloat > kViewportMinScale) ? mScaleMinFloat : kViewportMinScale;

Ditto

@@ +6945,5 @@
>        }
>      }
>      // Now convert the scale into device pixels per CSS pixel.
>      nsIWidget *widget = nsContentUtils::WidgetForDocument(this);
> +    CSSToLayoutDeviceScale pixelRatio(widget ? widget->GetDefaultScale() : 1.0f);

Followup for GetDefaultScale

@@ +6963,5 @@
>  
>      // Also recalculate the default zoom, if it wasn't specified in the metadata,
>      // and the width is specified.
>      if (mScaleStrEmpty && !mWidthStrEmpty) {
> +      CSSToScreenScale defScale(float(aDisplayWidth) / float(width));

'defaultScale'?

::: content/base/src/nsDocument.h
@@ +1409,5 @@
>  
>    // These member variables cache information about the viewport so we don't have to
>    // recalculate it each time.
>    bool mValidWidth, mValidHeight;
> +  mozilla::LayoutDeviceToScreenScale mScaleMinFloat, mScaleMaxFloat, mScaleFloat;

Looks like this just spills over 80 columns. I'd suggest

  mozilla::LayoutDeviceToScreenScale mScaleMinFloat;
  mozilla::LayoutDeviceToScreenScale mScaleMaxFloat;
  mozilla::LayoutDeviceToScreenScale mScaleFloat;

or a typedef to drop the mozilla::

::: content/base/src/nsViewportInfo.cpp
@@ +22,5 @@
>    // dev.w3.org/csswg/css-device-adapt section 6.2
>    mMaxZoom = std::max(mMinZoom, mMaxZoom);
>  
> +  mDefaultZoom = mDefaultZoom < mMaxZoom ? mDefaultZoom : mMaxZoom;
> +  mDefaultZoom = mDefaultZoom > mMinZoom ? mDefaultZoom : mMinZoom;

clamped probably works here too

::: dom/ipc/TabChild.cpp
@@ +625,5 @@
>    }
>  
>    minScale = mInnerSize.width / pageSize.width;
> +  minScale = clamped((double)minScale, (double)viewportInfo.GetMinZoom().scale,
> +                     (double)viewportInfo.GetMaxZoom().scale);

Try clamped with the typed scales
Attachment #796755 - Flags: review?(Ms2ger) → review+
Comment on attachment 796757 [details] [diff] [review]
Part 2 - Convert the viewport size

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

Thanks. Please move the changes I mentioned below into the previous patch or drop them. r=me with that.

::: content/base/public/nsViewportInfo.h
@@ +30,4 @@
>        mAutoSize(true),
>        mAllowZoom(true)
>      {
> +        mSize = mozilla::gfx::RoundedToInt(mozilla::ScreenSize(aDisplaySize) / mDefaultZoom);

Hrm, that's annoying, but I guess it's fine

@@ +30,5 @@
>        mAutoSize(true),
>        mAllowZoom(true)
>      {
> +        mSize = mozilla::gfx::RoundedToInt(mozilla::ScreenSize(aDisplaySize) / mDefaultZoom);
> +        mozilla::CSSToLayoutDeviceScale pixelRatio(1.0f);

This change doesn't belong in this patch.

@@ +37,4 @@
>          ConstrainViewportValues();
>      }
>  
> +    nsViewportInfo(const mozilla::CSSToScreenScale& aDefaultZoom,

Neither does this.

@@ +86,1 @@
>      // in CSS pixels.

I think that much is obvious now. Drop this line, please.

::: content/base/src/nsContentUtils.cpp
@@ +4850,5 @@
>  
>  /* static */
>  nsViewportInfo
>  nsContentUtils::GetViewportInfo(nsIDocument *aDocument,
> +                                const ScreenIntSize& aDisplaySize)

Wow, that's a pretty useless function.

::: content/base/src/nsDocument.cpp
@@ +6833,1 @@
>              return ret;

Just |return nsViewportInfo(aDisplaySize);|? (Pre-existing.)

@@ +6838,5 @@
>        nsAutoString handheldFriendly;
>        GetHeaderData(nsGkAtoms::handheldFriendly, handheldFriendly);
>        if (handheldFriendly.EqualsLiteral("true")) {
>          mViewportType = DisplayWidthHeight;
> +        nsViewportInfo ret(aDisplaySize);

Ditto

@@ +6951,5 @@
>  
>      if (mAutoSize) {
> +      // aDisplaySize is in screen pixels; convert them to CSS pixels for the viewport size.
> +      CSSToScreenScale defaultPixelScale = pixelRatio * LayoutDeviceToScreenScale(1.0f);
> +      size = mozilla::gfx::RoundedToInt(ScreenSize(aDisplaySize) / defaultPixelScale);

This is truncated right now, not rounded, right?

@@ +6956,3 @@
>      }
>  
> +    size.width = std::min(size.width, kViewportMaxSize.width);

mozilla::clamped? (Pre-existing)

@@ +6984,2 @@
>                         mAutoSize, mAllowZoom);
>      return ret;

Nit: |return nsViewportInfo(...);|

::: layout/base/nsPresShell.cpp
@@ +9608,5 @@
>    nsCOMPtr<nsIScreen> screen;
>    screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
>    if (screen) {
>      int32_t screenLeft, screenTop, screenWidth, screenHeight;
>      screen->GetRect(&screenLeft, &screenTop, &screenWidth, &screenHeight);

Followup for this.
Attachment #796757 - Flags: review?(Ms2ger) → review+
(In reply to :Ms2ger from comment #8)
> > +      CSSToScreenScale defaultPixelScale = pixelRatio * LayoutDeviceToScreenScale(1.0f);
> > +      size = mozilla::gfx::RoundedToInt(ScreenSize(aDisplaySize) / defaultPixelScale);
> 
> This is truncated right now, not rounded, right?
> 

Oh, I left this as rounded because after thinking about it I believe it is more correct.
https://hg.mozilla.org/mozilla-central/rev/ef69ff386624
https://hg.mozilla.org/mozilla-central/rev/610908ca3739
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.