1084500 - Refactor & expose some background-image positioning/sizing code

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug to cover some helper-patches that refactor & expose our background-image positioning/sizing code, so we can share them with our "object-fit" / "object-position" implementation. (bug 624647)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Refactor ComputeBackgroundAnchorPoint to take a Position & use a per-component helper method.

This part:
 (a) Refactors the function to take a nsStyleBackground::Position instead of a nsStyleBackground::Layer.  (This will be necessary because object-position uses a ::Position.)

 (b) Refactors the function to use a per-component helper, to avoid having copypasted logic / arithmetic.

 (c) In the helper function -- unlike in the code it replaces -- we check "aCoord.mHasPercent" *before* using the percent component. We don't strictly *need* to check mHasPercent, since we enforce that the mPercent component is 0.0f when mHasPercent is false. But it's nice hygenically to check the bool before using the thing that it guards, and it also lets us skip some unnecessary float math when mHasPercent is false. (Background note: aCoord is a "nsStyleCoord::CalcValue", which is typedef-aliased as nsStyleBackground::PositionCoord.  It contains a nscoord and an optional percent-value.)

Comment 2

[Daniel Holbert [:dholbert]]

forgot to mention:
 (d) Several minor tweaks to improve ComputeBackgroundAnchorPoint documentation, while I'm refactoring it.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: Promote & rename ComputeBackgroundAnchorPoint to nsImageRenderer::ComputeObjectAnchorPoint

This second patch promotes ComputeBackgroundAnchorPoint to a public static method, exposed for other code to use.  This patch also renames it (and its helper) with s/Background/Object/.  Finally, this patch moves the documentation (without modification) to the .h file.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Extend documentation to cover non-background use cases

Last part -- this tweaks the documentation for this method to cover replaced-element use cases (though nobody uses it for replaced elements yet, of course; usages will be added in bug 624647).

I also fixed a typo ("specify's"), made the indentation a bit saner for better readability, and added [out] annotations on the outparams.

Comment 5

[Daniel Holbert [:dholbert]]

Try run:
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b95f0caf7ed
 https://tbpl.mozilla.org/?tree=Try&rev=8b95f0caf7ed

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8507091 [details] [diff] [review]
part 1: Refactor ComputeBackgroundAnchorPoint to take a Position & use a per-component helper method.

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

Nice!

::: layout/base/nsCSSRendering.cpp
@@ +947,5 @@
> + * Helper for ComputeBackgroundAnchorPoint; parameters are the same as for
> + * that function, except they're for a single coordinate / a single size
> + * dimension. (so, x/width vs. y/height)
> + */
> +typedef nsStyleBackground::Position::PositionCoord PositionCoord;

Not-even-a-nit: You could express this as "using nsStyleBackground::Position::PositionCoord;", I think.

@@ +953,5 @@
> +ComputeBackgroundAnchorCoord(const PositionCoord& aCoord,
> +                             const nscoord aOriginBounds,
> +                             const nscoord aImageSize,
> +                             nscoord* aTopLeftCoord,
> +                             nscoord* aAnchorPointCoord)

Consider including "Out" in the names of these arguments, or something similar to unambiguously indicate that they're out-params.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8507112 [details] [diff] [review]
part 3: Extend documentation to cover non-background use cases

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

Looks good!

::: layout/base/nsCSSRendering.h
@@ +150,5 @@
> +   *          of the root element's frame. For a replaced element, this should
> +   *          be the element's content-box.
> +   * @param aTopLeft [out] The top-left corner where an image tile should be
> +   *          drawn.
> +   * @param aAnchorPoint [out] A point which should be pixel-aligned by

I'd probably still at least consider adding |Out| to the actual parameter names, as I mentioned in the review for part 1, but this certainly does make things clearer.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #6)
> Not-even-a-nit: You could express this as "using
> nsStyleBackground::Position::PositionCoord;", I think.

Thanks! I'll do that. I had this here to match a similar typedef in nsRuleNode, but 

> Consider including "Out" in the names of these arguments, or something
> similar to unambiguously indicate that they're out-params.

That's an interesting idea, but I think I'd lean against it, if it's OK with you. I don't think it's a general coding-style guideline that we follow in many places -- I only found 3 examples in existing code (only searching javadoc documentation, though, so not complete search)[1].

I *do* seem to recall someone suggesting a few years back that we should consider using a different prefix-letter (say, "o" instead of "a") for outparam arguments. If we were to change the style guide to indicate outparamness via variable-name, I think I'd prefer something along those lines instead of an "Out" suffix, for brevity.

In this case, I think it's pretty clear that the args are outparams, since:
 (a) At callsites: callers have to pass in a *pointer* to the outparam-args, which indicates that we're going to mess with those objects. E.g. in part 2, we have this at the call-site:
   nsImageRenderer::ComputeObjectAnchorPoint(aLayer.mPosition,
                                             bgPositionSize, imageSize,
                                             &imageTopLeft, &state.mAnchor);

 (b) In the documentation: we've got the [out] annotation in the javadoc text (added in part 3), making this clear

So, I think I'd rather leave this as-is, if you're all right with that.

[1] (My quick-and-dirty scan for "Out" suffixes in existing code was...
    grep -r "@param\s\+\S\+Out " $SRCDIR/*
...which turned up 2 instances in nsScriptLoader.h and 1 instance in nsFrameSelection.h, along with several other instances with an outparam named "aOut", which I'm not counting since that's not really a suffix. :)  This wasn't an exhaustive search, since it only covers javadoc-style "@param" documentation.)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Thanks! I'll do that. I had this here to match a similar typedef in
> nsRuleNode, but 

er, didn't finish my thought -- "...but I agree that a "using" decl is probably cleaner."

Comment 10

[Daniel Holbert [:dholbert]]

Alas, I can't replace the typedef with "using". If I try that, I get:
> nsCSSRendering.cpp:63:36: error: using declaration can not refer to class member
> using nsStyleBackground::Position::PositionCoord;
>       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

So, sticking with the typedef after all. (I guess that's why we've got a similar typedef in nsRuleNode.cpp.)

Comment 11

[Daniel Holbert [:dholbert]]

Landed patches:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/41f067463dd7
 https://hg.mozilla.org/integration/mozilla-inbound/rev/937bca227af8
 https://hg.mozilla.org/integration/mozilla-inbound/rev/df6729b4b6d8

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/41f067463dd7
https://hg.mozilla.org/mozilla-central/rev/937bca227af8
https://hg.mozilla.org/mozilla-central/rev/df6729b4b6d8

Comment 13

[yiorsi]

What time to implement background-repeat: [space | round]?

Comment 14

[Daniel Holbert [:dholbert]]

I don't know what our timeline is for those values -- sorry.  This particular bug was not about adding any new values -- it was just refactoring some existing code for usage elsewhere.

This bug is not really the right place to direct your question (or subsequent discussion). You'd be better off asking on mozilla's "dev.platform" or "dev.tech.layout" newsgroups (also available as mailing lists).  See https://www.mozilla.org/en-US/about/forums/ for more info.

Comment 15

[Daniel Holbert [:dholbert]]

(Oh, looks like that's bug 548372, which I guess Florian added as a "see also" for this bug, though I'm not clear why. Maybe because this bug shifted around some code that'll be used in that other bug?)