[prev in list] [next in list] [prev in thread] [next in thread] 

List:       webkit-reviews
Subject:    [webkit-reviews] review granted: [Bug 216012] Webpages flash when switching between windows : [Attac
From:       bugzilla-daemon () webkit ! org
Date:       2020-08-31 23:04:19
Message-ID: bug-216012-0-Bp3fWy7RQf () https ! bugs ! webkit ! org/
[Download RAW message or body]

Darin Adler <darin@apple.com> has granted Sihui Liu <sihui_liu@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@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.
_______________________________________________
webkit-reviews mailing list
webkit-reviews@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-reviews

[prev in list] [next in list] [prev in thread] [next in thread]