307532 - Selection colors should be saved in nsTextFrame::TextPaintStyle, not DrawSelectionIterator

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm working on bug 113161 and bug 170951.

In this work, I need selection colors in nsTextFrame::PaintTextDecorations. But
this function isn't using DrawSelectionIterator, but it has TextPaintStyle. I
think that DrawSelectionIterator should not have colors. It should refer
TextPaintStyle. And DrawSelectionIterator::GetSelectionColors should be moved to
TextPaintStyle. I think TextPaintStyle should be class, not struct.(struct can
have methods, but it is not beautiful. And I hate that the nsTextFrame
definition becomes long.)

I will create the patch tomorrow.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0(-u8pw)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

diff:

changing name; TextStyle -> nsTextStyle, TextPaintStyle -> nsTextPaintStyle.
removing color related members from DrawSelectionIterator.
removing some parameter from constructor of DrawSelectionIterator.
moving EnsureDifferentColors to previous in the code.

This patch solves the selection colors only once at first time of getting color
by GetSelectionColors. If GetSelectionColors is called by two or more times,
GetSelectionColors returns cached colors. Therefore, memory usage and
performance are better than current code.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 196357 [details] [diff] [review]
Patch rv1.0

nsTextFrame.cpp is updated by bug 306895. I will create new patch.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0 (-u8pw)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

Update to latest code.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1(-u8pw)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 200212 [details] [diff] [review]
Patch rv2.1

+  PRBool GetTextColor(nscolor* aTextColor);
+  PRBool GetSelectionColors(nscolor* foreColor,
+                            nscolor* backColor,
+                            PRBool*  aBackIsTransparent);

These don't need to return PRBool; it's a programming error if they ever fail (right?). So just put the assertions inside these functions and don't check in the caller. Then GetTextColor should return the color directly.

+  PRBool ShouldExchangeColors(nscolor *aForeColor, nscolor *aBackColor);

ShouldExchangeColors is not a good name, since it actually does the exchange if necessary. "Should" suggests that the function just tests whether the exchange is necessary. Call it something else like "EnsureSufficientContrast".

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for check-in

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Thanks.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Was this expected to affect Tp?  It improved by 2-4% or more on btek, luna, monkey (seamonkey tinderboxen, linux and mac), and got worse by about 20% on beast (Firefox tinderbox, windows, though there this patch is not the only one in the window).

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, if so, this may improve.
Because this patch doesn't process the "getting selection colors" if it's not needed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also, this caused the orange on balsa.  This patch is managing to leak enough stuff that we're asserting on shutdown, and asserts are fatal on balsa.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  We used to get the selection colors on every textframe paint?  And now we only get them if it's selected?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

this leaks the selection controller

In +nsTextFrame::GetSelectionStatus:
+  nsISelectionController* selectionController;
+  nsresult rv = GetSelectionController(aPresContext, &selectionController);

This addrefs, and never releases. fix is probably to make that an nsCOMPtr.

reopening. this causes the orange on balsa and probably enormous leaks.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: leak patch

this should fix things, though I didn't test it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201265 [details] [diff] [review]
leak patch

Looks good.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> Ah, ok.  We used to get the selection colors on every textframe paint?  And now
> we only get them if it's selected?
> 

If there are useless DrawSelectionIterator(s), you're right.

Biesi:

Thanks, and Sorry.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Tinderbox is green, now.

-> FIXED.

Thanks!

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, good.  Tp is also back where it should be with the leak fixed.