Closed Bug 1341156 Opened 8 years ago Closed 8 years ago

Support image border in Quantum Render.

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(4 files, 7 obsolete files)

Bug 1341156 - Modify webrender_ffi and add OpDPPushBorderImage.
59 bytes, text/x-review-board-request
ethlin
: review+
kats
: review+
Details
Bug 1341156 - Move nsImageRenderer to a separate file.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341156 - Create nsCSSBorderImageRenderer.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341156 - Add border image support.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
WebRender implementation in https://github.com/servo/webrender/pull/895 It's almost done. Then we can hook it up in gecko.
wr only supports image border now.
Summary: Support image and gradient border in Quantum Render. → Support image border in Quantum Render.
Assignee: nobody → mtseng
Attached patch wip (obsolete) — — Splinter Review
I'll do code cleanup and request review next. MozReview-Commit-ID: 5T3VlSfxDo0
Attachment #8840332 - Attachment is obsolete: true
Comment on attachment 8842307 [details] Bug 1341156 - Change nsDisplayOutline to match border api change. https://reviewboard.mozilla.org/r/116166/#review117740
Attachment #8842307 - Flags: review?(ethlin) → review+
Comment on attachment 8840788 [details] Bug 1341156 - Move nsImageRenderer to a separate file. https://reviewboard.mozilla.org/r/115222/#review118454
Attachment #8840788 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8840789 [details] Bug 1341156 - Create nsCSSBorderImageRenderer. https://reviewboard.mozilla.org/r/115224/#review118458 It's hard to be sure, but I'm assuming this just moves code around and won't have any functional changes.
Attachment #8840789 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8840790 [details] Bug 1341156 - Add border image support. https://reviewboard.mozilla.org/r/115226/#review118460 ::: layout/painting/nsDisplayList.h:2884 (Diff revision 2) > + RefPtr<imgIContainer> mImage; > + mozilla::LayerSize mImageSize; > + mozilla::Array<mozilla::LayerCoord, 4> mSlice; > + mozilla::Array<mozilla::LayerCoord, 4> mOutset; > + uint8_t mRepeatModeHorizontal; > + uint8_t mRepeatModeVertical; This is a lot of extra properties to have in BorderLayer when they aren't used frequently. Maybe use DisplayItemLayer for border-image? Or just a BorderImageLayer.
Attachment #8840790 - Flags: review?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #17) > This is a lot of extra properties to have in BorderLayer when they aren't > used frequently. > > Maybe use DisplayItemLayer for border-image? Or just a BorderImageLayer. I'll use DisplayItemLayer. And if we use DisplayyItemLayer, we don't need part 2 to send image container in border layer.
I'll fold part1 and part6 as well. Since without part6, part1 would build fail.
Attachment #8840786 - Attachment is obsolete: true
Attachment #8840786 - Flags: review?(jmuizelaar)
Attachment #8840787 - Attachment is obsolete: true
Attachment #8840787 - Flags: review?(nical.bugzilla)
Attachment #8840788 - Attachment is obsolete: true
Attachment #8840789 - Attachment is obsolete: true
Attachment #8840790 - Attachment is obsolete: true
Attachment #8842307 - Attachment is obsolete: true
Review board got confused when I push review from my hg instead of my git-cinnabar....
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #24) > Review board got confused when I push review from my hg instead of my > git-cinnabar.... Matt, I cannot carry r+ because above reason. Would you review it again? Thanks.
Comment on attachment 8843249 [details] Bug 1341156 - Modify webrender_ffi and add OpDPPushBorderImage. https://reviewboard.mozilla.org/r/117036/#review118684 Mostly this looks ok, but the NormalBorder/ImageBorder structs being passed over FFI are not repr(C) on the rust side, so I don't think that is safe. ::: gfx/webrender_bindings/WebRenderTypes.h:270 (Diff revision 1) > + return offset; > +} > +static inline WrSideOffsets2Df32 ToWrSideOffsets2Df32(float top, float right, float bottom, float left) Add a blank line between these ::: gfx/webrender_bindings/webrender_ffi.h:275 (Diff revision 1) > return top_left == aRhs.top_left && top_right == aRhs.top_right && > bottom_left == aRhs.bottom_left && bottom_right == aRhs.bottom_right; > } > }; > > +struct WrBorderWidth { This should be called WrBorderWidths (with "s" at the end) ::: gfx/webrender_bindings/webrender_ffi.h:288 (Diff revision 1) > + return left == aRhs.left && top == aRhs.top && > + right == aRhs.right && bottom == aRhs.bottom; > + } > +}; > + > +struct WrNormalBorder { Before you land this you have to make sure all the corresponding rust structs are tagged with repr(C), otherwise the layout can be anything, and you cannot safely pass this across the FFI boundary. So that means BorderWidths, BorderRadius, NormalBorder, ImageBorder, NinePatchDescriptor, RepeatMode in types.rs and SideOffsets2D in euclid all need to be tagged as repr(C). ::: layout/painting/nsDisplayList.cpp:4164 (Diff revision 1) > + WrBorderRadius borderRadius = > + wr::ToWrBorderRadius(LayerSize(br->mBorderRadii[0].width, br->mBorderRadii[0].height), > - LayerSize(br->mBorderRadii[1].width, br->mBorderRadii[1].height), > + LayerSize(br->mBorderRadii[1].width, br->mBorderRadii[1].height), > - LayerSize(br->mBorderRadii[3].width, br->mBorderRadii[3].height), > + LayerSize(br->mBorderRadii[3].width, br->mBorderRadii[3].height), > - LayerSize(br->mBorderRadii[2].width, br->mBorderRadii[2].height)); > + LayerSize(br->mBorderRadii[2].width, br->mBorderRadii[2].height)); This is pre-existing so you don't have to fix it in your patch but it would be great if we started using named constants instead of 0..3 for indexing these things. So for example br->mBorderRadii[eCornerTopLeft] instead of br->mBorderRadii[0].
Attachment #8843249 - Flags: review?(bugmail)
Comment on attachment 8843249 [details] Bug 1341156 - Modify webrender_ffi and add OpDPPushBorderImage. https://reviewboard.mozilla.org/r/117036/#review118684 > Add a blank line between these Done. > Before you land this you have to make sure all the corresponding rust structs are tagged with repr(C), otherwise the layout can be anything, and you cannot safely pass this across the FFI boundary. So that means BorderWidths, BorderRadius, NormalBorder, ImageBorder, NinePatchDescriptor, RepeatMode in types.rs and SideOffsets2D in euclid all need to be tagged as repr(C). I end up giving up send ImageBorder/NormalBorder across the ffi border. Just like before we have a struct with repr(C) in the bindings.rs and convert it to the correspond rust type in bindings.rs as well. > This is pre-existing so you don't have to fix it in your patch but it would be great if we started using named constants instead of 0..3 for indexing these things. So for example br->mBorderRadii[eCornerTopLeft] instead of br->mBorderRadii[0]. Done.
Comment on attachment 8843249 [details] Bug 1341156 - Modify webrender_ffi and add OpDPPushBorderImage. https://reviewboard.mozilla.org/r/117036/#review118774 LGTM, thanks! ::: commit-message-d6998:3 (Diff revision 2) > +MozReview-Commit-ID: Ct63gy6nk3q > +* * * > +commit 28f9cea112a2e246cb39302907fa9a7ab4cd5a51 > +Author: Morris Tseng <mtseng@mozilla.com> > + p1 > + Please fix commit message
Attachment #8843249 - Flags: review?(bugmail) → review+
This will interfere with the cleanups in bug 1344396. Would you be willing to rebase on top of it?
Comment on attachment 8843249 [details] Bug 1341156 - Modify webrender_ffi and add OpDPPushBorderImage. https://reviewboard.mozilla.org/r/117036/#review119020
Attachment #8843249 - Flags: review?(ethlin) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #33) > This will interfere with the cleanups in bug 1344396. Would you be willing > to rebase on top of it? Yes, I'll.
Depends on: 1344396
All patches have been rebased on bug 1344396.
Comment on attachment 8843250 [details] Bug 1341156 - Move nsImageRenderer to a separate file. https://reviewboard.mozilla.org/r/117038/#review120204
Attachment #8843250 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8843251 [details] Bug 1341156 - Create nsCSSBorderImageRenderer. https://reviewboard.mozilla.org/r/117040/#review120206
Attachment #8843251 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8843252 [details] Bug 1341156 - Add border image support. https://reviewboard.mozilla.org/r/117042/#review120208
Attachment #8843252 - Flags: review?(matt.woodrow) → review+
Pushed by mtseng@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/00f893bd8fdc Modify webrender_ffi and add OpDPPushBorderImage. r=kats r=ethlin https://hg.mozilla.org/projects/graphics/rev/f16631836b01 Move nsImageRenderer to a separate file. r=mattwoodrow https://hg.mozilla.org/projects/graphics/rev/558d790deba9 Create nsCSSBorderImageRenderer. r=mattwoodrow https://hg.mozilla.org/projects/graphics/rev/1b7a53f54686 Add border image support. r=mattwoodrow
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 1437625
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: