Bug 31468 - Unify TextBoundaries implementations by only relying on WTF Unicode abstractions
Summary: Unify TextBoundaries implementations by only relying on WTF Unicode abstractions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 15914 31469
  Show dependency treegraph
 
Reported: 2009-11-13 05:43 PST by Dominik Röttsches (drott)
Modified: 2009-11-14 03:57 PST (History)
1 user (show)

See Also:


Attachments
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (11.67 KB, patch)
2009-11-13 08:28 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v2) (11.70 KB, patch)
2009-11-13 08:43 PST, Dominik Röttsches (drott)
darin: review+
Details | Formatted Diff | Diff
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3) (11.73 KB, patch)
2009-11-13 09:13 PST, Dominik Röttsches (drott)
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2009-11-13 05:43:44 PST
Currently, there are multiple TextBoundaries implementations, one based on ICU (platform/text/TextBoundariesICU.cpp), one for QT (text/qt/TextBoundaries.cpp) - and for closing bug 15914, there would be another one needed for GTK/GLib. In order to reduce code duplication, I suggest to unify these into one, only using WTF abstractions.
Comment 1 Dominik Röttsches (drott) 2009-11-13 08:28:51 PST
Created attachment 43157 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp

TextBoundaries.cpp intends to be equivalent to the TextBoundariesICU.cpp but is relying on internal abstractions. 
Removing unneeded ICU includes in SimpleFontDataGtk.cpp on the way.

I did not try to merge the Qt version of TextBoundaries.cpp - could someone from the Qt WebKit team look at whether that's desired? Who would be best CC'ed on this bug regarding this question?
Comment 2 Dominik Röttsches (drott) 2009-11-13 08:43:34 PST
Created attachment 43160 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v2)

Minor copyright fix.
Comment 3 Darin Adler 2009-11-13 08:48:28 PST
Comment on attachment 43160 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v2)

We normally use "using namespace" to bring in the WTF Unicode functions. See this idiom in files such as Document.cpp, Font.cpp, and String.cpp.

This patch otherwise looks good to me.

r=me
Comment 4 Dominik Röttsches (drott) 2009-11-13 09:13:39 PST
Created attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Changed according to Darin's suggestion to use a "using namespace" declaration.
Comment 5 Eric Seidel (no email) 2009-11-13 12:30:52 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Changes to be committed also need an r+.   Looks sane to me too.
Comment 6 WebKit Commit Bot 2009-11-13 13:48:04 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Rejecting patch 43164 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11621 test cases.
http/tests/xmlhttprequest/access-control-basic-allow-access-control-origin-header-data-url.html -> crashed

Exiting early after 1 failures. 9149 tests run.
466.37s total testing time

9148 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 7 Eric Seidel (no email) 2009-11-13 13:52:40 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

We have a bad JSC regression atm.  bug 31461.  Let's try again.
Comment 8 WebKit Commit Bot 2009-11-13 14:11:33 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Rejecting patch 43164 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
ld/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCompositionEvent.o /Users/eseidel/Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSEventCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSEventCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(2 failures)
Comment 9 Eric Seidel (no email) 2009-11-13 14:13:52 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

I'm in the process of moving the commit-queue to a new machine, and it's had hiccups.  Sorry.
Comment 10 WebKit Commit Bot 2009-11-13 14:45:56 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Rejecting patch 43164 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
ld/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCompositionEvent.o /Users/eseidel/Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSEventCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSEventCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(2 failures)
Comment 11 Eric Seidel (no email) 2009-11-13 14:48:11 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

My continued apologies.  I've reverted back to the old commit-queue machine
until I can figure out the build issue.  Hopefully the next run will not hit
the JSC crasher. :(
Comment 12 WebKit Commit Bot 2009-11-13 15:47:57 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Rejecting patch 43164 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11622 test cases.
http/tests/xmlhttprequest/workers/shared-worker-close.html -> crashed

Exiting early after 1 failures. 9308 tests run.
522.97s total testing time

9307 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 13 Eric Seidel (no email) 2009-11-13 15:49:38 PST
Comment on attachment 43164 [details]
Refactoring TextBoundariesICU.cpp into generic TextBoundaries.cpp (v3)

Bug 31461 again. :(
Comment 14 Eric Seidel (no email) 2009-11-13 16:10:12 PST
Landed as r50977.
Comment 15 Eric Seidel (no email) 2009-11-13 16:39:49 PST
Committed r50980: <http://trac.webkit.org/changeset/50980>
Comment 16 Eric Seidel (no email) 2009-11-13 16:40:19 PST
Windows and Chromium builds broke after the first commit.  The second commit was to fix them.
Comment 17 Dominik Röttsches (drott) 2009-11-14 03:57:12 PST
Thanks for additional review and build fixes, Eric!