1328494 - Support nsDisplayBullet

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Comment 1

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

Attachment: Convert nsDisplayBullet to layer.

Currently, we only support list-style-image.

MozReview-Commit-ID: 8cDvUfwJqUF

Comment 2

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

Comment on attachment 8824342 [details] [diff] [review]
Convert nsDisplayBullet to layer.

Text layer were supported. I'll update my patch to support text part. Remove the review flag.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8824342 [details] [diff] [review]
Convert nsDisplayBullet to layer.

Review of attachment 8824342 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBulletFrame.cpp
@@ +294,5 @@
> +    return LAYER_NONE;
> +  }
> +
> +  RefPtr<DrawTargetCapture> capture =
> +    gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget()->CreateCaptureDT(IntSize());

Why do we create a capture DT when we don't actually use the recording for anything?

It looks like it's only used to allocate a PathBuilder, which just redirects the call back to the reference DT (ScreenReferenceDrawTarget).

What happens if the painting DT isn't the same format as the screen reference one?

Maybe instead we could just allocate a PathBuilderRecording (which isn't specific to any backend), and then convert to the 'real' format using PathRecording::StreamToSink.

Comment 4

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

Thanks for the comment. I'll use PathBuilderRecording in next patch.

Comment 5

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8824342 [details] [diff] [review]
> Convert nsDisplayBullet to layer.
> 
> Review of attachment 8824342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsBulletFrame.cpp
> @@ +294,5 @@
> > +    return LAYER_NONE;
> > +  }
> > +
> > +  RefPtr<DrawTargetCapture> capture =
> > +    gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget()->CreateCaptureDT(IntSize());
> 
> Why do we create a capture DT when we don't actually use the recording for
> anything?
> 
> It looks like it's only used to allocate a PathBuilder, which just redirects
> the call back to the reference DT (ScreenReferenceDrawTarget).
> 
> What happens if the painting DT isn't the same format as the screen
> reference one?
> 
> Maybe instead we could just allocate a PathBuilderRecording (which isn't
> specific to any backend), and then convert to the 'real' format using
> PathRecording::StreamToSink.

Can I create a PathSkia directly? I look into PathBuilderRecording and PathRecording. I found that PathRecording is a wrapper of another path class which means PathRecording has a member "mPath" reference to another path. And PathRecording::StreamToSink just called mPath->StreamToSink. Thus, we don't need PathRecording to accomplish our job.

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #5)

> Can I create a PathSkia directly? I look into PathBuilderRecording and
> PathRecording. I found that PathRecording is a wrapper of another path class
> which means PathRecording has a member "mPath" reference to another path.
> And PathRecording::StreamToSink just called mPath->StreamToSink. Thus, we
> don't need PathRecording to accomplish our job.

Yeah, good catch.

We should probably use ScreenReferenceDrawTarget to create the PathBuilder, and then when we try draw we can use StreamToSink if the types don't match.

Comment 7

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

Attachment: Convert nsDisplayBullet to layer.

MozReview-Commit-ID: 8cDvUfwJqUF

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8826494 [details] [diff] [review]
Convert nsDisplayBullet to layer.

Review of attachment 8826494 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: layout/generic/nsBulletFrame.cpp
@@ +171,5 @@
>    }
>  #endif
>  }
>  
> +class BulletRenderer final

It's not obvious that most of these members are mutually exclusive without reading the code using it.

Could we instead make all the members private, and add multiple constructors that construct that various 'types' (image, path, text) of BulletRenderer?

We could then make BuildLayer/PaintBullet member function of BulletRenderer.

Please also add comments/assertions that the various members are only used the right way.

@@ +335,5 @@
> +    if (!container) {
> +      return nullptr;
> +    }
> +
> +    RefPtr<layers::ImageLayer> layer = static_cast<layers::ImageLayer*>

It seems plausible that we created a TexrLayer last paint, and a content change means that we now want an ImageLayer, so this static_cast could be invalid.

We should call GetLeafLayerFor outside of the if() block, and then check if the type of layer matches what we need for this current paint.

@@ +362,5 @@
> +    layer->SetBaseTransform(gfx::Matrix4x4::Translation(offset.x, offset.y, 0));
> +    return layer.forget();
> +  }
> +
> +  if (!mBulletRenderer->mText.IsEmpty()) {

Splitting image and text layers out into separate helper functions is probably worthwhile, this is pretty long.

@@ +397,5 @@
> +    std::vector<Glyph> glyph;
> +    RefPtr<ScaledFont> font;
> +    Color color;
> +    if (!capture->ContainsOnlyColoredGlyphs(font, color, glyph)) {
> +      return nullptr;

What happens here? We just won't render the bullet now?

Comment 9

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> @@ +397,5 @@
> > +    std::vector<Glyph> glyph;
> > +    RefPtr<ScaledFont> font;
> > +    Color color;
> > +    if (!capture->ContainsOnlyColoredGlyphs(font, color, glyph)) {
> > +      return nullptr;
> 
> What happens here? We just won't render the bullet now?

In the next patch, I'll split the process of build text layer into two parts. One is build glyph by using DrawTargetCapture and another one is actually creating the text layer. And I'll call build glyph in GetLayerState. If the build process is fail, return LAYERS_NONE which means we don't using text layer to render it.

Comment 10

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

Attachment: Convert nsDisplayBullet to layer.

Addressed reviewer's comment.

MozReview-Commit-ID: 8cDvUfwJqUF

Comment 11

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

Attachment: Convert nsDisplayBullet to layer.

Fix crash.

MozReview-Commit-ID: 8cDvUfwJqUF

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8829346 [details] [diff] [review]
Convert nsDisplayBullet to layer.

Review of attachment 8829346 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBulletFrame.cpp
@@ +230,5 @@
> +    if (IsImageType()) {
> +      return BuildLayerForImage(aBuilder, aManager, aContainerParameters, aItem);
> +    }
> +
> +    if (IsPathType()) {

We can't be multiple types, so make these else if()s.

The last one can just be else() with MOZ_ASSERT(IsTextType()), then you don't need unreachable.

@@ +454,5 @@
> +    bool dummy;
> +    const LayoutDeviceIntRect destBounds =
> +      LayoutDeviceIntRect::FromAppUnitsToOutside(aItem->GetBounds(aBuilder, &dummy), A2D);
> +    layer->SetBounds(IntRect(destBounds.x, destBounds.y, destBounds.width, destBounds.height));
> +    layer->SetBaseTransform(gfx::Matrix4x4::Translation(aContainerParameters.mOffset.x,

SetBaseTransform and GetLeafLayerFor could probably move out to the caller instead of repeating them.

Comment 13

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

Attachment: Convert nsDisplayBullet to layer. (carry r+: mattwoodrow)

Addressed reviewer's commnet.

MozReview-Commit-ID: 8cDvUfwJqUF

Comment 14

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30c6ee335bf1

Comment 15

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

try looks positive.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e465b90bf8e8
Convert nsDisplayBullet to layer. r=mattwoodrow

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e465b90bf8e8