Bug 123355 – Move InlineTextBox's text painting to its own class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Drawing text isn't modular

[Myles C. Maxfield]

Created attachment 215201 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/15322614>

[Myles C. Maxfield]

Created attachment 215206 [details]
Patch

[Build Bot]

Comment on attachment 215206 [details]
Patch

Attachment 215206 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/11948005

[Myles C. Maxfield]

Created attachment 215214 [details]
Patch

[Myles C. Maxfield]

Created attachment 215328 [details]
Patch

[Dean Jackson]

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.

[Myles C. Maxfield]

Created attachment 215431 [details]
Patch

[Dean Jackson]

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.

[WebKit Commit Bot]

Comment on attachment 215431 [details]
Patch

Clearing flags on attachment: 215431

Committed r158232: <http://trac.webkit.org/changeset/158232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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.