Refactor & expose some background-image positioning/sizing code

RESOLVED FIXED in mozilla36

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla36
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
Created attachment 8507091 [details] [diff] [review]
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.)
(Assignee)

Updated

3 years ago
Attachment #8507091 - Flags: review?(seth)
(Assignee)

Comment 2

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

Comment 3

3 years ago
Created attachment 8507099 [details] [diff] [review]
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.
Attachment #8507099 - Flags: review?(seth)
(Assignee)

Comment 4

3 years ago
Created attachment 8507112 [details] [diff] [review]
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.
Attachment #8507112 - Flags: review?(seth)
(Assignee)

Comment 5

3 years ago
Try run:
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b95f0caf7ed
 https://tbpl.mozilla.org/?tree=Try&rev=8b95f0caf7ed
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.
Attachment #8507091 - Flags: review?(seth) → review+
Attachment #8507099 - Flags: review?(seth) → review+
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.
Attachment #8507112 - Flags: review?(seth) → review+
(Assignee)

Comment 8

3 years ago
(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.)
(Assignee)

Comment 9

3 years ago
(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."
(Assignee)

Comment 10

3 years ago
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.)
(Assignee)

Comment 11

3 years ago
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
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/41f067463dd7
https://hg.mozilla.org/mozilla-central/rev/937bca227af8
https://hg.mozilla.org/mozilla-central/rev/df6729b4b6d8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Updated

3 years ago
See Also: → bug 548372

Comment 13

3 years ago
What time to implement background-repeat: [space | round]?
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Comment 15

3 years ago
(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?)
You need to log in before you can comment on or make changes to this bug.