376180 - convert nsBulletFrame to draw using gfxContext

Status: RESOLVED WORKSFORME

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Joey Minta]

Attachment: patch v1

The attached patch converts nsBulletFrame to draw bullets using a gfxContext rather than nsIRenderingContext, in line with what stuart and vlad outlined at the dev day.  It also removes an unnecessary include from nsPageContentFrame.cpp.

One possible issue: NS_STYLE_LIST_STYLE_CIRCLEs appear thinner than in FF2, I tried playing around with SetLineWidth(), but never got results that I was happy with.  I don't know (a) how wide the circles are supposed to be and (b) a better way to set that width.  Help/clarification appreciated.

Note that after this patch we still use nsIRenderingContext to DrawString(), because I haven't figured out text/fonts at all yet.

Comment 1

[Joey Minta]

Attachment: screenshot comparison of bullets

Side-by-side of FF2 bullets and bullets with this patch.

Comment 2

[Martijn Wargers (dead)]

Vlad, is the patch still valid? Could you review it?

Comment 3

[Zack Weinberg (:zwol)]

Vlad suggested I look at this patch as a jumping off point for working on nsIRenderingContext removals.  A couple of notes:-

1) Is this really the right way to get the gfxContext from the passed-in nsIRenderingContext?  It's not typesafe.  Perhaps GetNativeGraphicData should be split up into several functions, one for each of its possible return types?

    nsRefPtr<gfxContext> ctx = (gfxContext*)aRenderingContext.GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);

2) I note that Thebes is not the sole implementation of nsIRenderingContext currently in the tree, and this code will blow up horribly with the alternatives.  Probably nobody cares whether the BeOS and Photon builds still work, but then perhaps all that code should be nuked from the tree?

3) I suspect that the difference in bullets is down to floating-point rounding differences.  It's the same math before and after but the operations are in a different order.  Will try to fix tomorrow.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> Vlad suggested I look at this patch as a jumping off point for working on
> nsIRenderingContext removals.  A couple of notes:-
> 
> 1) Is this really the right way to get the gfxContext from the passed-in
> nsIRenderingContext?  It's not typesafe.  Perhaps GetNativeGraphicData should
> be split up into several functions, one for each of its possible return types?
> 
>     nsRefPtr<gfxContext> ctx =
> (gfxContext*)aRenderingContext.GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);

I added ThebesContext() on nsIRC a while ago, so it should be used instead of GetNativeGraphicData.  GNGD was a long-time hack for various things, we shouldn't really be changing this interface any for cleanup purposes since it's going away.

> 2) I note that Thebes is not the sole implementation of nsIRenderingContext
> currently in the tree, and this code will blow up horribly with the
> alternatives.  Probably nobody cares whether the BeOS and Photon builds still
> work, but then perhaps all that code should be nuked from the tree?

Yes, eventually; there are various removal patches kicking around, it hasn't been important, and that code has been a useful reference (though not so useful any more).  Just ignore it.

> 3) I suspect that the difference in bullets is down to floating-point rounding
> differences.  It's the same math before and after but the operations are in a
> different order.  Will try to fix tomorrow.

Great!

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #4)
> I added ThebesContext() on nsIRC a while ago, so it should be used instead of
> GetNativeGraphicData.

Just what I was looking for, thanks.

> > 2) I note that Thebes is not the sole implementation of nsIRenderingContext
> Just ignore it.

Ok.

FYI, since this is clearly not going into FF3 I'm going to work relative to mozilla-central rather than CVS trunk.

Comment 6

[Zack Weinberg (:zwol)]

Additional question for you, Vlad: Do you have any suggestions for writing a test case for the correct rendering of the bullet circles?  The only thing I can think of is to use a reftest whose reference rendering uses list-style-image, but that would trip over normal variation across platforms.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: revised patch with no change in bullet presentation

Here is a revised patch which does the coordinate conversion exactly the same way the old code did and therefore draws the bullets the same way the old code did.

I looked into using the gfxContext to draw text bullets (i.e. counters), but for text, nontrivial work is being done within the nsRenderingContext and the nsFontMetrics classes, still.  nsTextFrameThebes appears to duplicate a bunch of that.  I'd prefer to refactor that down into gfx/thebes first.

More generally, I'm wondering whether we ought to make nsThebesRenderingContext a totally empty husk of a wrapper class *first*.  No state of its own.  Otherwise, we're risking having the gfxContext state and the nsThebesRenderingContext state get out of sync with each other.  (Not to mention the nsRenderingContextImpl state and the nsFontMetricsImpl state ...)

Also, I think the nscoord to gfxFloat unit conversion ought to be encapsulated in helper functions (essentially, publicize the FROM_TWIPS macros that are currently in nsThebesRenderingContext). dbaron objected tp sprinkling divisions by AppUnitsPerDevPixel() all over layout.  [Actually, could we get away with redefining nscoord *as* gfxFloat, such that AppUnitsPerDevPixel was always 1?)

Comment 8

[Zack Weinberg (:zwol)]

Attachment: testcase exercising lots of different bullet types

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Actually, could we get away with redefining nscoord *as* gfxFloat

See bug 265084 for some background on that.

Comment 10

[Zack Weinberg (:zwol)]

Thanks for the pointer.

You probably read the thing I put on the wiki, so I'll just mention here that I now think having gfxFloat be in app units is the better way forward.

Comment 11

[Zack Weinberg (:zwol)]

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

Comment 12

[Wayne Mery (:wsmwk)]

is this still a thing?
it is the only remaining blocker to bug 430829

Comment 13

[Sean Voisen (:svoisen)]

I believe we'd want to finish the work on bug 1542807 first, which I think would make this bug moot. Bug 1542807 is on our roadmap as an upcoming project.

Comment 14

[Mats Palmgren (inactive)]

nsBulletFrame was removed in bug 1542807.