[webkit-reviews] review denied: [Bug 199848] [WebKit] Add PageLoadState::Observer C API : [Attachment 374758] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 29 10:11:39 PDT 2019

Alex Christensen <achristensen at apple.com> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 199848: [WebKit] Add PageLoadState::Observer C API

Attachment 374758: Patch


--- Comment #9 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 374758
  --> https://bugs.webkit.org/attachment.cgi?id=374758

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

Please add a unit test in TestWebKitAPI that verifies these are actually
called.  Just adding a change to MiniBrowser and hoping we would notice any
regressions is not sufficient regression protection.

> Source/WebKit/UIProcess/API/APIStateClient.h:32
> +class StateClient {

This is duplicate with PageLoadState::Observer.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2306
> +	   void willChangeIsLoading() override

I would make the class final or make these overrides final. It's not too big of
a deal in this case because it's a function scoped class, but it's something we
try to do.

> Source/WebKit/UIProcess/WebPageLoadStateObserver.h:36
> +    WebPageLoadStateObserver(std::unique_ptr<API::StateClient>&& client)

This should be UniqueRef if it's never null.

More information about the webkit-reviews mailing list