Closed Bug 1614198 Opened 4 years ago Closed 4 years ago

Remove nsStyleImage.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files)

Bug 1614198 - Update cbindgen. r=#style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1614198 - Merge ImageLayer and Image. r=#style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1614198 - Make CachedBorderImageData a frame property. r=tnikkel,aosmond,#style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1614198 - Some preparation to start using cbindgen for shape-outside and clip-path. r=boris,#style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1614198 - Use cbindgen instead of nsStyleImage. r=#style,tnikkel,aosmond
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

I need this to support individual #[cfg] in enum variants, which is used for
Image::PaintWorklet.

ImageLayer is almost the only usage of Image, so keeping them in the same enum
makes the resulting C++ struct smaller, and makes it map more cleanly to
nsStyleImage.

Depends on D62160

Storing it in the style value doesn't make much sense, specially since we store
drawing-dependent stuff there like the svg viewport size, which clearly depends
on the size of the frame.

This cache may not be very useful anymore, see the linked bug, but this unblocks
me with other style system cleanups and seems safer.

Depends on D62161

Tweak the ShapeSourceRepresentation so that it doesn't store Option<>s.

Some renames so that GeometryBox doesn't conflict with the Gecko type, and some
other usual bits / re-exports to deal with cbindgen and generics.

Also, drive-by derive parsing of GeometryBox as it's trivial.

Doing this unfortunately is not possible without removing nsStyleImage first, so
let's do that before.

This makes us serialize in the shortest form for shape-outside, but that's what
we should do anyway.

(aside: the shapes code is a bit too generic, maybe we should unify
ClippingShape and FloatAreaShape...)

Depends on D62162

The trickier part is that we represent -moz-image-rect as a Rect() type instead
of image with non-null clip-rect. So we need to add a bit of code to
distinguish "image request types" from other types of images.

But it's not too annoying, and we need to do the same for fancier images like
image-set and such whenever we implement it, so seems nice to get rid of
most explicit usages of nsStyleImage::GetType().

Depends on D62163

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

This cache may not be very useful anymore, see the linked bug, but this unblocks me with other style system cleanups and seems safer.

Which bug?

Flags: needinfo?(emilio)

How can we determine whether this cache is useful or not? I would rather remove it if, as you say, it might not be very useful any more, particularly as this change will make us hit the cache less (as each frame will have its own cache, rather than hopefully hitting the rule cache or style sharing cache across elements).

(In reply to Cameron McCormack (:heycam) from comment #8)

How can we determine whether this cache is useful or not? I would rather remove it if, as you say, it might not be very useful any more, particularly as this change will make us hit the cache less (as each frame will have its own cache, rather than hopefully hitting the rule cache or style sharing cache across elements).

Bug 1564526 (it's referenced from a comment).

It is mostly hit for tiling border images at this point, afaict, and even with that we may not be re-rasterizing (so we're just creating a cheap wrapper object).

That bug has links to more history. I'd rather not kill it just yet, but I do want to get back to that bug and measure it again, I'm sure I did do that talos try push and then forgot to check the results :(

But from my conversations with Andrew before filing that bug, I think most if not all of the performance issues that cache was working around have been fixed in imagelib.

Flags: needinfo?(emilio)

I wonder how much of the potential sharing-across-frames is really sound, too... Seems to me those images are frame-size-dependent, right? At least the border slice depends on mImageSize, which depends on mArea, which depends on aBorderArea which is just the frame size.

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3695dec8b66
Merge ImageLayer and Image. r=heycam
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b33496ddb1d9
Merge ImageLayer and Image. r=heycam
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/3eda66fbf7bb
Remove trait bound and comment that snuck from a previous patch accidentally in a CLOSED TREE
Blocks: 1614510
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae402b74010d
Make CachedBorderImageData a frame property. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/c97286e92b25
Some preparation to start using cbindgen for shape-outside and clip-path. r=boris
https://hg.mozilla.org/integration/autoland/rev/31f468c8c960
Use cbindgen instead of nsStyleImage. r=aosmond
Keywords: leave-open
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21738 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: