Bug 177493

Summary:

Extract out combined text query into a member function

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

Layout and Rendering

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, hyatt, mmaxfield, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

175784, 177495

Attachments:

Description	Flags
Patch	none
Patch	none

Daniel Bates

Reported 2017-09-26 10:52:52 PDT

Extract out combined text query into a member function.

Attachments
Patch (15.87 KB, patch) 2017-09-26 11:09 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Patch (15.87 KB, patch) 2017-09-26 11:18 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Daniel Bates

Comment 1 2017-09-26 11:09:17 PDT

Created attachment 321844 [details] Patch

Daniel Bates

Comment 2 2017-09-26 11:18:51 PDT

Created attachment 321848 [details] Patch

zalan

Comment 3 2017-09-26 12:56:20 PDT

Comment on attachment 321848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321848&action=review > Source/WebCore/rendering/InlineTextBox.cpp:1117 > + return lineStyle().hasTextCombine() && is<RenderCombineText>(renderer()) && downcast<RenderCombineText>(renderer()).isCombined() ? &downcast<RenderCombineText>(renderer()) : nullptr; Not sure why we check both the style and the type of the renderer here. If the style has text combine, the renderer is supposed to be RenderCombineText (and vice versa). Also do you mind checking if fontToUse() could take advantage of this change -it's a static function though).

Daniel Bates

Comment 4 2017-09-26 15:27:28 PDT

(In reply to zalan from comment #3) > Comment on attachment 321848 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321848&action=review > > > Source/WebCore/rendering/InlineTextBox.cpp:1117 > > + return lineStyle().hasTextCombine() && is<RenderCombineText>(renderer()) && downcast<RenderCombineText>(renderer()).isCombined() ? &downcast<RenderCombineText>(renderer()) : nullptr; > > Not sure why we check both the style and the type of the renderer here. If > the style has text combine, the renderer is supposed to be RenderCombineText > (and vice versa). This was strange to me as well when I was performing the refactor. This patch is just moving code. I hope you do not mind that defer this investigation to another bug. > Also do you mind checking if fontToUse() could take advantage of this change -it's a static function though). Yes, I plan to update fontToUse() to take advantage of this change. I plan to do this in bug #177495.

Daniel Bates

Comment 5 2017-09-26 15:28:48 PDT

Comment on attachment 321848 [details] Patch Clearing flags on attachment: 321848 Committed r222528: <http://trac.webkit.org/changeset/222528>

Daniel Bates

Comment 6 2017-09-26 15:28:49 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 7 2017-09-27 12:15:33 PDT

<rdar://problem/34692884>

Note You need to log in before you can comment on or make changes to this bug.