nsHTMLCanvasFrame::ComputeSize has duplicated intrinsic size code

RESOLVED FIXED in Firefox 68

Status

()

task
P3
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a month ago

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.)

Assignee

Comment 1

a month ago

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

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

Comment 2

a month ago

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.)

Assignee

Comment 3

a month ago

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.

Comment 4

a month ago
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

Comment 5

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.