Closed Bug 1339683 Opened 3 years ago Closed 3 years ago

Convert nsDisplayBullet to DisplayItemLayer.

Categories

(Core :: Graphics: WebRender, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54

People

(Reporter: mtseng, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

This will convert nsDisplayBullet to DisplayItemLayer.
Attachment #8837431 - Flags: review?(matt.woodrow)
Attachment #8837432 - Flags: review?(matt.woodrow)
Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

https://reviewboard.mozilla.org/r/112560/#review114222

This looks ok to me, but punting the actual SendImageContainer code to nical
Attachment #8837429 - Flags: review?(mchang) → review+
Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

Can you please take a look at WebRenderDisplayItemLayer::SendImageContainer?
Attachment #8837429 - Flags: review?(nical.bugzilla)
Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

https://reviewboard.mozilla.org/r/112562/#review114226

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp:76
(Diff revision 2)
> +  uint32_t mIndex;
> +  float mGlyphSize;
> +};
> +
> +static void
> +DWriteFontFileData(const uint8_t* aData, uint32_t aLength, uint32_t aIndex,

nit: This was renamed in a patch that I pushed yesterday.

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp:97
(Diff revision 2)
> +WebRenderDisplayItemLayer::CreateWebRenderCommandsFromGlyph(nsTArray<WebRenderCommand>& aCommands,
> +                                                            const nsTArray<GlyphArray>& aGlyphs,
> +                                                            gfx::ScaledFont* aFont,
> +                                                            const gfx::IntRect& aBounds)
> +{
> +  MOZ_ASSERT(aFont);

This is mostly copy / pasted from the WebRenderTextLayer. Can we not duplicate this code? e.g. pass in aOffset and use this code in WebRenderTextLayer as well please?
(In reply to Mason Chang [:mchang] from comment #12)
> Comment on attachment 8837430 [details]
> Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.
> This is mostly copy / pasted from the WebRenderTextLayer. Can we not
> duplicate this code? e.g. pass in aOffset and use this code in
> WebRenderTextLayer as well please?

Yap, next patch would remove the dup code. Thanks for the comment.
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #18)
> (In reply to Mason Chang [:mchang] from comment #12)
> > Comment on attachment 8837430 [details]
> > Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.
> > This is mostly copy / pasted from the WebRenderTextLayer. Can we not
> > duplicate this code? e.g. pass in aOffset and use this code in
> > WebRenderTextLayer as well please?
> 
> Yap, next patch would remove the dup code. Thanks for the comment.

Can we please fix in this patch?
Flags: needinfo?(mtseng)
(In reply to Mason Chang [:mchang] from comment #19)
> Can we please fix in this patch?

Yap, I mean the next version of the patch. I have uploaded the newer version of patch that removed the dup code. You can see the latest review request.
Flags: needinfo?(mtseng)
Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

https://reviewboard.mozilla.org/r/112562/#review114690

::: gfx/layers/wr/WebRenderLayerManager.cpp:43
(Diff revision 3)
> +  float mGlyphSize;
> +};
> +
> +static void
> +WriteFontFileData(const uint8_t* aData, uint32_t aLength, uint32_t aIndex,
> +                  float aGlyphSize, uint32_t aVariationCount,

I don't think this data should live in a WebRenderLayer. Maybe pass in a struct or something? I was ok with having it be a separate utility function, I was just saying WebRenderTextLayer should use that utility function.
Comment on attachment 8837431 [details]
Bug 1339683 - Move the function definition out of class for readability.

https://reviewboard.mozilla.org/r/112564/#review114750
Attachment #8837431 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8837432 [details]
Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

https://reviewboard.mozilla.org/r/112566/#review114752

I don't understand the rectangles given to WR entirely, you might want someone else to review that bit.

::: layout/generic/nsBulletFrame.cpp:614
(Diff revision 3)
> +BulletRenderer::CreateWebRenderCommandsForPath(nsDisplayItem* aItem,
> +                                               nsTArray<layers::WebRenderCommand>& aCommands,
> +                                               layers::WebRenderLayer* aLayer)
> +{
> +  MOZ_ASSERT(IsPathType());
> +  // Not supported yet.

MOZ_ASSERT_UNREACHABLE?
Attachment #8837432 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8837433 [details]
Bug 1339683 - Remove build layer codes which are not used anymore.

https://reviewboard.mozilla.org/r/112568/#review114754
Attachment #8837433 - Flags: review?(matt.woodrow) → review+
(In reply to Mason Chang [:mchang] from comment #21)
> Comment on attachment 8837430 [details]
> Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.
> 
> https://reviewboard.mozilla.org/r/112562/#review114690
> 
> ::: gfx/layers/wr/WebRenderLayerManager.cpp:43
> (Diff revision 3)
> > +  float mGlyphSize;
> > +};
> > +
> > +static void
> > +WriteFontFileData(const uint8_t* aData, uint32_t aLength, uint32_t aIndex,
> > +                  float aGlyphSize, uint32_t aVariationCount,
> 
> I don't think this data should live in a WebRenderLayer. Maybe pass in a
> struct or something? I was ok with having it be a separate utility function,
> I was just saying WebRenderTextLayer should use that utility function.

I moved the utility function to gfxUtils. Please review it again. tks.
Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

https://reviewboard.mozilla.org/r/112560/#review114842
Attachment #8837429 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

https://reviewboard.mozilla.org/r/112562/#review114964

Thanks!
Attachment #8837430 - Flags: review?(mchang) → review+
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/193a8185498a
Supporting send ImageContainer in the DisplayItemLayer. r=mchang r=nical
https://hg.mozilla.org/projects/graphics/rev/06746b37e4b1
Utility function to create WebRenderCommands from Glyphs. r=mchang
https://hg.mozilla.org/projects/graphics/rev/0cd4548db0f2
Move the function definition out of class for readability. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/4ffd45a1bb75
Add CreateWebRenderCommands for nsDisplayBullet. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/d9cf30343904
Remove build layer codes which are not used anymore. r=mattwoodrow
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1342343
Comment on attachment 8837432 [details]
Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

https://reviewboard.mozilla.org/r/112566/#review118098

::: layout/generic/nsBulletFrame.cpp:600
(Diff revision 4)
> +  Rect destRect =
> +    NSRectToRect(mDest, appUnitsPerDevPixel);
> +  Rect destRectTransformed = aLayer->RelativeToParent(destRect);
> +  IntRect dest = RoundedToInt(destRectTransformed);
> +
> +  aCommands.AppendElement(layers::OpDPPushExternalImageId(

It seems like this id won't be valid if we replay these commands without calling layer->SendImageContainer()
Flags: needinfo?(mtseng)
It still valid, SendImageContainer just return the cached mExternalImageId to the caller. But if the content of image is changed, you need call SendImageContainer again to update image in the parent side. Otherwise, the parent will get the previous image. Whether the image is change or not, replay the commands is always valid since we don't change the external image id after the first SendImageContainer is called.
Flags: needinfo?(mtseng)
You need to log in before you can comment on or make changes to this bug.