Closed Bug 1328494 Opened 7 years ago Closed 7 years ago

Support nsDisplayBullet

Categories

(Core :: Graphics: WebRender, defect)

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(1 file, 4 obsolete files)

      No description provided.
Currently, we only support list-style-image.

MozReview-Commit-ID: 8cDvUfwJqUF
Attachment #8824342 - Flags: review?(matt.woodrow)
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.
Attachment #8824342 - Flags: review?(matt.woodrow)
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.
Thanks for the comment. I'll use PathBuilderRecording in next patch.
(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.
Flags: needinfo?(matt.woodrow)
(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.
Flags: needinfo?(matt.woodrow)
MozReview-Commit-ID: 8cDvUfwJqUF
Attachment #8826494 - Flags: review?(matt.woodrow)
Attachment #8824342 - Attachment is obsolete: true
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?
(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.
Addressed reviewer's comment.

MozReview-Commit-ID: 8cDvUfwJqUF
Attachment #8827697 - Flags: review?(matt.woodrow)
Attachment #8826494 - Attachment is obsolete: true
Attachment #8826494 - Flags: review?(matt.woodrow)
Fix crash.

MozReview-Commit-ID: 8cDvUfwJqUF
Attachment #8829346 - Flags: review?(matt.woodrow)
Attachment #8827697 - Attachment is obsolete: true
Attachment #8827697 - Flags: review?(matt.woodrow)
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.
Attachment #8829346 - Flags: review?(matt.woodrow) → review+
Addressed reviewer's commnet.

MozReview-Commit-ID: 8cDvUfwJqUF
Attachment #8829753 - Flags: review+
Attachment #8829346 - Attachment is obsolete: true
try looks positive.
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e465b90bf8e8
Convert nsDisplayBullet to layer. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/e465b90bf8e8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: