Bug 120891

Summary:

Internals should always cause a layout before calling into TextIterator

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Tools / Tests

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue, darin, esprehn, kling, koivisto

Priority:

Keywords:

BlinkMergeCandidate

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

120685

Attachments:

Description	Flags
Fixes Internals	none
Force a layout in TextIterator's constructor	koivisto: review+, commit-queue: commit-queue-

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	Details Formatted Diff Diff
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-	Details Formatted Diff Diff
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.