Bug 128797 - setSelection should not synchronously trigger layout
Summary: setSelection should not synchronously trigger layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 127832
  Show dependency treegraph
 
Reported: 2014-02-13 22:17 PST by Ryosuke Niwa
Modified: 2014-02-14 14:22 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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>