Bug 123355

Summary: Move InlineTextBox's text painting to its own class
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, enrica, esprehn+autocc, glenn, gyuyoung.kim, jonlee, koivisto, kondapallykalyan, mitz, rakuco, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Myles C. Maxfield 2013-10-25 13:08:51 PDT
Drawing text isn't modular
Comment 1 Myles C. Maxfield 2013-10-25 13:15:17 PDT
Created attachment 215201 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2013-10-25 13:16:12 PDT
<rdar://problem/15322614>
Comment 3 Myles C. Maxfield 2013-10-25 13:34:26 PDT
Created attachment 215206 [details]
Patch
Comment 4 Build Bot 2013-10-25 14:08:46 PDT
Comment on attachment 215206 [details]
Patch

Attachment 215206 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/11948005
Comment 5 Myles C. Maxfield 2013-10-25 14:47:51 PDT
Created attachment 215214 [details]
Patch
Comment 6 Myles C. Maxfield 2013-10-28 13:41:18 PDT
Created attachment 215328 [details]
Patch
Comment 7 Dean Jackson 2013-10-29 14:48:18 PDT
Comment on attachment 215328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215328&action=review

Looks good, just needs license change.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:11736
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug_WinCairo|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug_WinCairo|x64'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='DebugSuffix|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='DebugSuffix|x64'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|x64'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release_WinCairo|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release_WinCairo|x64'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Production|Win32'">true</ExcludedFromBuild>
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Production|x64'">true</ExcludedFromBuild>

I'm not sure we need all this crap. I tend to leave it out and see what breaks.

> Source/WebCore/rendering/TextPainter.cpp:18
> + * Copyright (C) 2013 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *

This is not the right license. Look for a recently added file by an apple contributor and copy it from there. e.g. Source/WebKit/ios/WebCoreSupport/WebInspectorClientIOS.mm

> Source/WebCore/rendering/TextPainter.h:18
> + * Copyright (C) 2013 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *

Ditto.
Comment 8 Myles C. Maxfield 2013-10-29 14:55:20 PDT
Created attachment 215431 [details]
Patch
Comment 9 Dean Jackson 2013-10-29 14:56:48 PDT
Comment on attachment 215431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215431&action=review

> Source/WebCore/ChangeLog:21
> +        * CMakeLists.txt: Adding new TextPainter class
> +        * GNUmakefile.list.am: Adding new TextPainter class
> +        * WebCore.vcxproj/WebCore.vcxproj: Adding new TextPainter class
> +        * WebCore.vcxproj/WebCore.vcxproj.filters: Adding new TextPainter
> +        class
> +        * WebCore.xcodeproj/project.pbxproj: Adding new TextPainter class

People tend to just say "Ditto" here.
Comment 10 WebKit Commit Bot 2013-10-29 15:27:44 PDT
Comment on attachment 215431 [details]
Patch

Clearing flags on attachment: 215431

Committed r158232: <http://trac.webkit.org/changeset/158232>
Comment 11 WebKit Commit Bot 2013-10-29 15:27:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Antti Koivisto 2013-10-29 15:40:50 PDT
Comment on attachment 215431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215431&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:576
> +    TextPainter textPainter(*context, paintSelectedTextOnly, paintSelectedTextSeparately, font, sPos, ePos, length, emphasisMark, combinedText, textRun, boxRect, textOrigin, emphasisMarkOffset, textShadow, selectionShadow, isHorizontal(), textPaintStyle, selectionPaintStyle);

This is bit excessive and confusing. It would be better to use a struct for passing this many arguments.