Closed
Bug 1339683
Opened 8 years ago
Closed 8 years ago
Convert nsDisplayBullet to DisplayItemLayer.
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mtseng, Unassigned)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
mchang
:
review+
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mchang
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
This will convert nsDisplayBullet to DisplayItemLayer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8837431 -
Flags: review?(matt.woodrow)
Attachment #8837432 -
Flags: review?(matt.woodrow)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-review |
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 11•8 years ago
|
||
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 12•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 18•8 years ago
|
||
(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•8 years ago
|
||
(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)
Reporter | ||
Comment 20•8 years ago
|
||
(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 21•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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 23•8 years ago
|
||
mozreview-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 24•8 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 30•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review |
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 32•8 years ago
|
||
mozreview-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+
Reporter | ||
Comment 33•8 years ago
|
||
Comment 34•8 years ago
|
||
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: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Target Milestone: --- → mozilla54
Comment 35•8 years ago
|
||
bugherder |
Comment 36•8 years ago
|
||
mozreview-review |
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()
Updated•8 years ago
|
Flags: needinfo?(mtseng)
Reporter | ||
Comment 37•8 years ago
|
||
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.
Description
•