Bug 123038 - Refactor LineBreaker::nextSegmentBreak, add BreakingContext that holds all its state
Summary: Refactor LineBreaker::nextSegmentBreak, add BreakingContext that holds all it...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks: 121261
  Show dependency treegraph
 
Reported: 2013-10-18 14:17 PDT by Zoltan Horvath
Modified: 2013-10-22 23:48 PDT (History)
8 users (show)

See Also:


Attachments
bullet for EWS bots (84.11 KB, patch)
2013-10-18 14:22 PDT, Zoltan Horvath
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (457.79 KB, application/zip)
2013-10-18 15:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (471.41 KB, application/zip)
2013-10-18 15:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (471.39 KB, application/zip)
2013-10-18 16:38 PDT, Build Bot
no flags Details
2nd bullet for EWSs (86.17 KB, patch)
2013-10-18 17:57 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (160.29 KB, patch)
2013-10-22 15:30 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (160.28 KB, patch)
2013-10-22 16:20 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (160.24 KB, patch)
2013-10-22 22:56 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2013-10-18 14:17:50 PDT
I tried the follow Levi's logic on Blink's nextSegmentBreak refactoring (https://chromiumcodereview.appspot.com/25054004). I mostly did the same changes, but the code is too diverged at this point to just apply that patch on our trunk.
The patch introduces BreakingContext as a separate class. I added new methods for each condition. I also removed the goto-s from the code.  All the new methods are inline in order to avoid introducing any performance regression. The change makes the code so much cleaner and understandable.
Comment 1 Zoltan Horvath 2013-10-18 14:22:59 PDT
Created attachment 214602 [details]
bullet for EWS bots
Comment 2 WebKit Commit Bot 2013-10-18 14:24:41 PDT
Attachment 214602 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlockFlow.h', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/shapes/ShapeInsideInfo.h']" exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1468:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1471:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2013-10-18 15:13:51 PDT
Comment on attachment 214602 [details]
bullet for EWS bots

Attachment 214602 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6448005

New failing tests:
editing/selection/after-line-wrap.html
Comment 4 Build Bot 2013-10-18 15:13:53 PDT
Created attachment 214608 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2013-10-18 15:33:36 PDT
Comment on attachment 214602 [details]
bullet for EWS bots

Attachment 214602 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6428012

New failing tests:
editing/selection/after-line-wrap.html
Comment 6 Build Bot 2013-10-18 15:33:38 PDT
Created attachment 214609 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2013-10-18 16:38:38 PDT
Comment on attachment 214602 [details]
bullet for EWS bots

Attachment 214602 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6398021

New failing tests:
editing/selection/after-line-wrap.html
Comment 8 Build Bot 2013-10-18 16:38:40 PDT
Created attachment 214617 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Zoltan Horvath 2013-10-18 17:57:24 PDT
Created attachment 214626 [details]
2nd bullet for EWSs
Comment 10 Dave Hyatt 2013-10-22 14:17:27 PDT
Comment on attachment 214626 [details]
2nd bullet for EWSs

r=me
Comment 11 Zoltan Horvath 2013-10-22 15:30:26 PDT
Created attachment 214893 [details]
Patch
Comment 12 Zoltan Horvath 2013-10-22 16:20:58 PDT
Created attachment 214901 [details]
Patch
Comment 13 WebKit Commit Bot 2013-10-22 16:57:55 PDT
Comment on attachment 214901 [details]
Patch

Rejecting attachment 214901 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 214901, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
57827 = e94c2c982ba63ffbf3cdb559e1b4ce761a2bd1ee
r157828 = 12840dc1a7bdcec0407f02b3f48f65c65cc4fd4a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp
r157829 = b113269736701107fca2829964c8b46b7e565d96 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/9428030
Comment 14 Zoltan Horvath 2013-10-22 18:25:32 PDT
I'm updating the patch after r157828.
Comment 15 Zoltan Horvath 2013-10-22 22:56:32 PDT
Created attachment 214931 [details]
Patch
Comment 16 WebKit Commit Bot 2013-10-22 23:48:34 PDT
Comment on attachment 214931 [details]
Patch

Clearing flags on attachment: 214931

Committed r157851: <http://trac.webkit.org/changeset/157851>
Comment 17 WebKit Commit Bot 2013-10-22 23:48:38 PDT
All reviewed patches have been landed.  Closing bug.