[webkit-reviews] review granted: [Bug 216012] Webpages flash when switching between windows : [Attachment 407632] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 216012: Webpages flash when switching between windows
https://bugs.webkit.org/show_bug.cgi?id=216012

Attachment 407632: Patch

https://bugs.webkit.org/attachment.cgi?id=407632&action=review




--- Comment #19 from Darin Adler <darin at apple.com> ---
Comment on attachment 407632
  --> https://bugs.webkit.org/attachment.cgi?id=407632
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407632&action=review

Can we find a way to make a regression test for this? We want this fixed for
the long term, not just fixed now and then broken in the future. Tests the main
way we do that.

We also need a test that would have detected our mistake before where
m_shouldHandleActivityStateChangeCallbacks was never set back to false.

> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:144
> +	   auto strongPage = makeRefPtr(weakThis->m_webPage);

If we’re going to be dereferencing this without null checking, then I suggest
putting this into a Ref rather than a RefPtr.

    auto strongPage = makeRef(*weakThis->m_webPage);

> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:145
> +	   auto* drawingArea =
static_cast<TiledCoreAnimationDrawingArea*>(strongPage->drawingArea());

If we’re going to be dereferencing this without null checking, then I suggest
putting this into a reference rather than a pointer:

    auto& drawingArea =
static_cast<TiledCoreAnimationDrawingArea&>(*strongPage->drawingArea());

> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:155
> +	   for (const auto& callbackID :
drawingArea->m_nextActivityStateChangeCallbackIDs)

Should just use auto here, not const auto&. Seems fine to copy an ID.