1339683 - Convert nsDisplayBullet to DisplayItemLayer.

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

This will convert nsDisplayBullet to DisplayItemLayer.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

Allow to create an ImageClient for ImageContainer and send this
ImageClient to WebRender for rendering.

Review commit: https://reviewboard.mozilla.org/r/112560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112560/

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

Review commit: https://reviewboard.mozilla.org/r/112562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112562/

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1339683 - Move the function definition out of class for readability.

r=mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/112564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112564/

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

r=mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/112566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112566/

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1339683 - Remove build layer codes which are not used anymore.

Review commit: https://reviewboard.mozilla.org/r/112568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112568/

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112562/diff/1-2/

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837431 [details]
Bug 1339683 - Move the function definition out of class for readability.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112564/diff/1-2/

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837432 [details]
Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112566/diff/1-2/

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837433 [details]
Bug 1339683 - Remove build layer codes which are not used anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112568/diff/1-2/

Comment 10

[Mason Chang [Inactive] [:mchang]]

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

Comment 11

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

Can you please take a look at WebRenderDisplayItemLayer::SendImageContainer?

Comment 12

[Mason Chang [Inactive] [:mchang]]

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?

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112560/diff/1-2/

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112562/diff/2-3/

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837431 [details]
Bug 1339683 - Move the function definition out of class for readability.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112564/diff/2-3/

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837432 [details]
Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112566/diff/2-3/

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837433 [details]
Bug 1339683 - Remove build layer codes which are not used anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112568/diff/2-3/

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 19

[Mason Chang [Inactive] [:mchang]]

(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?

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 21

[Mason Chang [Inactive] [:mchang]]

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 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8837431 [details]
Bug 1339683 - Move the function definition out of class for readability.

https://reviewboard.mozilla.org/r/112564/#review114750

Comment 23

[Matt Woodrow (:mattwoodrow)]

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?

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8837433 [details]
Bug 1339683 - Remove build layer codes which are not used anymore.

https://reviewboard.mozilla.org/r/112568/#review114754

Comment 25

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112560/diff/2-3/

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112562/diff/3-4/

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837431 [details]
Bug 1339683 - Move the function definition out of class for readability.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112564/diff/3-4/

Comment 28

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837432 [details]
Bug 1339683 - Add CreateWebRenderCommands for nsDisplayBullet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112566/diff/3-4/

Comment 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8837433 [details]
Bug 1339683 - Remove build layer codes which are not used anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112568/diff/3-4/

Comment 30

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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 31

[Nicolas Silva [:nical]]

Comment on attachment 8837429 [details]
Bug 1339683 - Supporting send ImageContainer in the DisplayItemLayer.

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

Comment 32

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8837430 [details]
Bug 1339683 - Utility function to create WebRenderCommands from Glyphs.

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

Thanks!

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7a6d41303f1

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/193a8185498a
https://hg.mozilla.org/mozilla-central/rev/06746b37e4b1
https://hg.mozilla.org/mozilla-central/rev/0cd4548db0f2
https://hg.mozilla.org/mozilla-central/rev/4ffd45a1bb75
https://hg.mozilla.org/mozilla-central/rev/d9cf30343904

Comment 36

[Jeff Muizelaar [:jrmuizel]]

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()

Comment 37

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.