Closed Bug 446498 Opened 16 years ago Closed 16 years ago

use new border rendering for focus rectangles, and strip dead code

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: zwol, Assigned: zwol)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch (rev 1) initial (obsolete) — Splinter Review
This is a tidy-up in preparation for addressing bug 19963 / bug 379303. The attached patch changes focus rectangles to be drawn with the new border renderer, which means we can throw away the old nsCSSRendering::DrawDashedSides() functions, and strips some dead code out of the new border renderer - the last three arguments to PaintBorders() and the last argument to PaintOutline() were never used (and only partially implemented).

Compiles cleanly and appears to have no visible effect on rendering; will run tests overnight.

I am assuming this is dbaron's territory.
Attachment #330676 - Flags: superreview?(dbaron)
Attachment #330676 - Flags: review?(dbaron)
Is perhaps worth mentioning that the changes to nsLayoutUtils.cpp in the patch are an unrelated, en passant warning fix - fallout from bug 437335.
I think Vlad should probably review this.
Comment on attachment 330676 [details] [diff] [review]
(rev 1) initial

Ok.
Attachment #330676 - Flags: superreview?(vladimir)
Attachment #330676 - Flags: superreview?(dbaron)
Attachment #330676 - Flags: review?(vladimir)
Attachment #330676 - Flags: review?(dbaron)
I'm going to make your patch rotted as soon as the tree reopens :(  Shouldn't be a hard port, though.
If you send me the patch(es) you mean to land, I can stick it in my queue and respin.
Same content, just revised to apply to the current tree.  Am running tests presently.

I audited the entire tree and found no uses of the aGap, aHardBorderSize, or aShouldIgnoreRounded arguments to nsCSSRendering::DrawBorder / DrawOutline.
Attachment #330676 - Attachment is obsolete: true
Attachment #330992 - Flags: superreview?(vladimir)
Attachment #330992 - Flags: review?(vladimir)
Attachment #330676 - Flags: superreview?(vladimir)
Attachment #330676 - Flags: review?(vladimir)
Comment on attachment 330992 [details] [diff] [review]
(rev 2) updated after bug 424423 landing


This all looks great, though one question:


>+void
>+nsCSSRendering::PaintFocus(nsPresContext* aPresContext,
>+                           nsIRenderingContext& aRenderingContext,
>+                           const nsRect& aFocusRect,
>+                           nscolor aColor)
>+{
...

Could this just draw a rectangle and stroke it, instead of going through all the border painting machinery?  Seems overkill for hardcoded all-border-widths-same and dotted borders.  I don't think it'll be all that much faster, though, but might be cleaner. Hrm, and might actually be slower for large elements unless you drew each side separately.. blah, ok, maybe not worth doing it.
Attachment #330992 - Flags: superreview?(vladimir)
Attachment #330992 - Flags: superreview+
Attachment #330992 - Flags: review?(vladimir)
Attachment #330992 - Flags: review+
The main reason I want to send focus rectangles through the border paint machinery is so I can reuse (yet to be written) code from there that ensures there is a dot at each corner.  I mean to give all-four-sides-dotted its own short circuit logic, since the special corner handling you need is rather different; it will (probably) draw and stroke one line for each, using the zero-length-dashes-with-rounded-endcaps thing that cairo guarantees will work.  I don't really know cairo performance but it seems to me that that oughta be as fast as it can go.
Keywords: checkin-needed
http://hg.mozilla.org/index.cgi/mozilla-central/rev/35b9d43cf9d5
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: