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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=199848

Attachment 374758: Patch

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




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

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.