[webkit-reviews] review requested: [Bug 137679] REGRESSION (r169024): Undetermined text is not displayed in the search field of Adobe Help Website : [Attachment 239886] with release build fix

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has asked  for review:
Bug 137679: REGRESSION (r169024): Undetermined text is not displayed in the
search field of Adobe Help Website
https://bugs.webkit.org/show_bug.cgi?id=137679

Attachment 239886: with release build fix
https://bugs.webkit.org/attachment.cgi?id=239886&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Turns out that we need to call computeTextSelectionPaintStyle even if we are
not going to use the style, as the function also initializes other variables.

> This doesn't make much sense - why should underlines dictate what the paint
style of the text above it is? Perhaps we should rename the useCustomUnderlines
variable.

Do you have a proposal for a better name? The variable is just a result of
calling compositionUsesCustomUnderlines(), so the name seems reasonable to me -
it doesn't tell us what the value is supposed to be used for, but it tells us
what it is.

> Are you sure you don't want to instead pass these variables to
computeTextSelectionPaintStyle instead?

Yes, I considered this, and preferred to keep both useCustomUnderlines checks
in the same function. We already use it in paint() like this:

	if (haveSelection && !useCustomUnderlines)
	    paintSelection(context, boxOrigin, lineStyle, font,
selectionPaintStyle.fillColor);

The logic is indeed somewhat unexpected, so it seems better to have it local in
one function, not spread all over the code.