[webkit-reviews] review granted: [Bug 128797] setSelection should not synchronously trigger layout : [Attachment 224171] Removed the stale declaration of selectionUpdateTimerFired

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 128797: setSelection should not synchronously trigger layout
https://bugs.webkit.org/show_bug.cgi?id=128797

Attachment 224171: Removed the stale declaration of selectionUpdateTimerFired
https://bugs.webkit.org/attachment.cgi?id=224171&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.