Bug 128797

Summary:

setSelection should not synchronously trigger layout

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, commit-queue, darin, enrica, hyatt, koivisto, simon.fraser

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

127832

Attachments:

Description	Flags
Work in progress	none
Removed Xcode change (still WIP)	none
Removes the sync layout	none
Removed the stale declaration of selectionUpdateTimerFired	koivisto: review+

Ryosuke Niwa

Reported 2014-02-13 22:17:23 PST

When there is a pending layout or style recalc, simply delay the work until the layout happens.

Attachments
Work in progress (15.04 KB, patch) 2014-02-13 22:21 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Removed Xcode change (still WIP) (11.19 KB, patch) 2014-02-13 22:26 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Removes the sync layout (10.04 KB, patch) 2014-02-14 00:06 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Removed the stale declaration of selectionUpdateTimerFired (9.61 KB, patch) 2014-02-14 00:10 PST, Ryosuke Niwa	koivisto: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2014-02-13 22:21:25 PST

Created attachment 224160 [details] Work in progress

WebKit Commit Bot

Comment 2 2014-02-13 22:23:27 PST

Attachment 224160 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 3 2014-02-13 22:26:21 PST

Created attachment 224161 [details] Removed Xcode change (still WIP)

WebKit Commit Bot

Comment 4 2014-02-13 22:28:13 PST

Attachment 224161 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 5 2014-02-14 00:06:03 PST

Created attachment 224170 [details] Removes the sync layout

Ryosuke Niwa

Comment 6 2014-02-14 00:06:31 PST

I'm quite happy about the fact I've successfully avoid adding yet-another timer.

Ryosuke Niwa

Comment 7 2014-02-14 00:10:27 PST

Created attachment 224171 [details] Removed the stale declaration of selectionUpdateTimerFired

Antti Koivisto

Comment 8 2014-02-14 12:29:21 PST

Comment on attachment 224171 [details] Removed the stale declaration of selectionUpdateTimerFired View in context: https://bugs.webkit.org/attachment.cgi?id=224171&action=review > Source/WebCore/dom/Document.cpp:1786 > +bool Document::needsLayout() const The name is confusing, this checks ancestor documents too. Something like selfOrParentDocumentNeedsLayout() perhaps? > Source/WebCore/dom/Document.h:558 > + bool needsStyleRecalc() const { return m_optimizedStyleSheetUpdateTimer.isActive() || m_pendingStyleRecalcShouldForce || childNeedsStyleRecalc(); } Does this really need to be inlined? Why isn't hasPendingStyleRecalc() sufficient? These are confusing enough already, I wouldn't like to add more slightly different variants. Why doesn't this need to recurse to ancestors? It is used in the same places as the newly added needsLayout(). > Source/WebCore/editing/FrameSelection.cpp:1338 > + if (!m_frame) Frameless FrameSelection sounds weird.

Ryosuke Niwa

Comment 9 2014-02-14 14:22:26 PST

Committed r164133: <http://trac.webkit.org/changeset/164133>

Note You need to log in before you can comment on or make changes to this bug.