[webkit-reviews] review granted: [Bug 136729] Latching in iframes is not working as expected : [Attachment 238322] Fixed crash in destructor call

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 136729: Latching in iframes is not working as expected
https://bugs.webkit.org/show_bug.cgi?id=136729

Attachment 238322: Fixed crash in destructor call
https://bugs.webkit.org/attachment.cgi?id=238322&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238322&action=review


> Source/WebCore/page/LatchedState.h:38
> +class LatchedState {

I think this should be called ScrollLatchingState. Out of context, it's hard to
know what is being latched.

You could also move it to page/scrolling.

> Source/WebCore/page/MainFrame.cpp:38
> +    , m_latchedState(std::make_unique<LatchedState>())

Should platforms without scroll events waste memory on a latched state?

> Source/WebCore/page/MainFrame.cpp:46
> +    m_latchedState = nullptr;
> +    m_recentWheelEventDeltaTracker = nullptr;

Neither is necessary.

> Source/WebCore/page/MainFrame.cpp:83
> +    if (!m_latchedState)
> +	   return;

This should never happen, right?

> Source/WebCore/page/MainFrame.h:48
> +    bool hasLatchedState() { return m_latchedState.get(); }
> +    LatchedState& latchedState() { return *m_latchedState; }

I don't see the advantage over just returning a pointer.

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:16
> +	       // The IFrame should have scrolled, but not the main page.

IFrame -> iframe everywhere.

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:34
> +	       var iFrameBody = window.frames['target'].document.body;

iFrame -> iframe