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+

Description Ryosuke Niwa 2014-02-13 22:17:23 PST

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

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

Created attachment 224160 [details]
Work in progress

Comment 2 WebKit Commit Bot 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.

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

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

Comment 4 WebKit Commit Bot 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.

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

Created attachment 224170 [details]
Removes the sync layout

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

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

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

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

Comment 8 Antti Koivisto 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.

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

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