RESOLVED FIXED 120891
Internals should always cause a layout before calling into TextIterator 
https://bugs.webkit.org/show_bug.cgi?id=120891
Summary Internals should always cause a layout before calling into TextIterator
Ryosuke Niwa
Reported 2013-09-06 13:13:08 PDT
Merge https://chromium.googlesource.com/chromium/blink/+/5fee5da7b04a710171c79bd6e87eca3533188e45 Calling into TextIterator without causing a layout first doesn't work and will break once we lazy attach everywhere. This fixes lots of tests that break once we lazy attach inside the parser.
Attachments
Fixes Internals (2.08 KB, patch)
2013-09-06 13:15 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Force a layout in TextIterator's constructor (3.87 KB, patch)
2013-09-06 14:02 PDT, Ryosuke Niwa 		koivisto: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2013-09-06 13:15:29 PDT
Created attachment 210787 [details] Fixes Internals
Alexey Proskuryakov
Comment 2 2013-09-06 13:21:46 PDT
Comment on attachment 210787 [details] Fixes Internals View in context: https://bugs.webkit.org/attachment.cgi?id=210787&action=review > Source/WebCore/ChangeLog:11 > + TextIterator expects the layout to be up-to-date. Force updating the layout in these Internals methods > + to avoid problems in the future such as intermittent test failures. We also call the same from WebKit, and don't update the layout. It would seem inconsistent if we only did the in Internals.
Darin Adler
Comment 3 2013-09-06 13:23:28 PDT
Comment on attachment 210787 [details] Fixes Internals View in context: https://bugs.webkit.org/attachment.cgi?id=210787&action=review >> Source/WebCore/ChangeLog:11 >> + to avoid problems in the future such as intermittent test failures. > > We also call the same from WebKit, and don't update the layout. It would seem inconsistent if we only did the in Internals. I’m not sure what your point is. Calling without updating layout is a bug. Why do we need to replicate that bug in our test harness? Would doing that help us make certain kinds of tests?
Darin Adler
Comment 4 2013-09-06 13:24:30 PDT
Comment on attachment 210787 [details] Fixes Internals View in context: https://bugs.webkit.org/attachment.cgi?id=210787&action=review >>> Source/WebCore/ChangeLog:11 >>> + to avoid problems in the future such as intermittent test failures. >> >> We also call the same from WebKit, and don't update the layout. It would seem inconsistent if we only did the in Internals. > > I’m not sure what your point is. Calling without updating layout is a bug. Why do we need to replicate that bug in our test harness? Would doing that help us make certain kinds of tests? By the way, sounds like we need to fix that WebKit bug! We should see whether we can reproduce a problem and fix the code whether we can or not.
Darin Adler
Comment 5 2013-09-06 13:25:36 PDT
Comment on attachment 210787 [details] Fixes Internals View in context: https://bugs.webkit.org/attachment.cgi?id=210787&action=review >>>> Source/WebCore/ChangeLog:11 >>>> + to avoid problems in the future such as intermittent test failures. >>> >>> We also call the same from WebKit, and don't update the layout. It would seem inconsistent if we only did the in Internals. >> >> I’m not sure what your point is. Calling without updating layout is a bug. Why do we need to replicate that bug in our test harness? Would doing that help us make certain kinds of tests? > > By the way, sounds like we need to fix that WebKit bug! We should see whether we can reproduce a problem and fix the code whether we can or not. Maybe Alexey is suggesting that TextIterator itself be responsible for calling updateLayout?
Alexey Proskuryakov
Comment 6 2013-09-06 13:57:27 PDT
I don't know if/why layout needs to be manually updated (I simply don't understand how this works). But if it does, then it definitely seems that this should be done by TextIterator itself.
Ryosuke Niwa
Comment 7 2013-09-06 14:02:38 PDT
Created attachment 210797 [details] Force a layout in TextIterator's constructor
WebKit Commit Bot
Comment 8 2013-09-07 10:18:04 PDT
Comment on attachment 210797 [details] Force a layout in TextIterator's constructor Rejecting attachment 210797 [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-03', 'apply-attachment', '--no-update', '--non-interactive', 210797, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: with fuzz 3. patching file Source/WebCore/editing/TextIterator.cpp Hunk #4 FAILED at 2596. 1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/editing/TextIterator.cpp.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/mac/editing/input/caret-primary-bidi-expected.txt Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Antti Koivisto']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/1719377
Ryosuke Niwa
Comment 9 2013-09-09 13:27:33 PDT
Committed r155378: <http://trac.webkit.org/changeset/155378>
Note You need to log in before you can comment on or make changes to this bug.