nsHTMLCanvasFrame::ComputeSize has duplicated intrinsic size code

RESOLVED FIXED in Firefox 68

Status

()

task
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

nsHTMLCanvasFrame::ComputeSize right now has this chunk of code:

  nsIntSize size = GetCanvasSize();

  IntrinsicSize intrinsicSize;
  intrinsicSize.width.SetCoordValue(
      nsPresContext::CSSPixelsToAppUnits(size.width));
  intrinsicSize.height.SetCoordValue(
      nsPresContext::CSSPixelsToAppUnits(size.height));

This is equivalent to just calling nsHTMLCanvasFrame::GetIntrinsicSize() (which is currently the one-liner return IntrinsicSizeFromCanvasSize(GetCanvasSize()), and IntrinsicSizeFromCanvasSize looks basically identical to the code quoted above.

Let's just call GetIntrinsicSize to simplify things. (GetIntrinsicSize is a virtual function, but both the caller and callee here are in nsHTMLCanvasFrame's own function-implementations, and nsHTMLCanvasFrame is final, so compilers should be able to devirtualize the call.)

(Motivation: this will make for fewer special cases in bug 1544121.)

Assignee: nobody → dholbert
Blocks: 1544121
Status: NEW → ASSIGNED
Type: defect → task

Actually, since we're getting both the intrinsic ratio and size in this function, we should pull out the GetCanvasSize() call so that we only call that part once, and use the Intrinsic[Size,Ratio]FromCanvasSize helpers on the result of that.

(This is slightly more optimal than calling GetIntrinsicSize();GetIntrinsicRatio(); would be, because each of those have a GetCanvasSize() call internally.)

This patch should not affect behavior; the new implementation is
identical to the old one, but with better sharing of code.

Also: I'm removing the code-comment saying that the intrinsic ratio
is unused, because it's not really useful and it's unclear to me that
it's strictly true. There are several cases in the function we pass it
to, nsFrame::ComputeSizeWithIntrinsicDimensions, that use the ratio
and that look reachable as long as we have 'width:auto' in CSS.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ed35343aea0
Refactor nsHTMLCanvasFrame::ComputeSize to use existing helper functions and avoid redundant GetCanvasSize() call. r=TYLin
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.