Bug 177493 - Extract out combined text query into a member function
Summary: Extract out combined text query into a member function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 175784 177495
  Show dependency tree / graph
 
Reported: 2017-09-26 10:52 PDT by Daniel Bates
Modified: 2017-09-27 12:15 PDT (History)
6 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-09-26 10:52:52 PDT 
Extract out combined text query into a member function.
Comment 1 Daniel Bates 2017-09-26 11:09:17 PDT 
Created attachment 321844 [details]
Patch
Comment 2 Daniel Bates 2017-09-26 11:18:51 PDT 
Created attachment 321848 [details]
Patch
Comment 3 zalan 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).
Comment 4 Daniel Bates 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.
Comment 5 Daniel Bates 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>
Comment 6 Daniel Bates 2017-09-26 15:28:49 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-09-27 12:15:33 PDT 
<rdar://problem/34692884>