Bug 199848 – [WebKit] Add PageLoadState::Observer C API

• Component: WebKit2
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][WebKit] Add PageLoadState::Observer C API

WKPageLoaderClientV0::didReceiveTitleForFrame has been deprecated
in r235398. There is no WebKit C API to get the timing of title
changed. Cocoa and glib WebKit API exist.

[Fujii Hironori]

Created attachment 374275 [details]
Patch

[Fujii Hironori]

Created attachment 374280 [details]
Patch

[Alex Christensen]

Comment on attachment 374280 [details]
Patch

There's nothing windows specific about this.  Why not make it cross-platform?

[Fujii Hironori]

Agreed. Will do so.

[Fujii Hironori]

Created attachment 374591 [details]
Patch

[Fujii Hironori]

Created attachment 374755 [details]
Patch

[Fujii Hironori]

Created attachment 374758 [details]
Patch

[Fujii Hironori]

r?

[Alex Christensen]

Comment on attachment 374758 [details]
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.

[Fujii Hironori]

Created attachment 375146 [details]
Patch

* Addressed review feedbacks

[Fujii Hironori]

Created attachment 375147 [details]
Patch

[Alex Christensen]

Comment on attachment 375147 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:639
> +void WebPageProxy::setPageLoadStateObserver(UniqueRef<PageLoadState::Observer>&& observer)

My earlier feedback was mistaken.  We ought to be able to call WKPageSetPageStateClient with nullptr and have it clear the observer.  That would bring this back to a std::unique_ptr.  Please do that, add that to the test, and r=me.

[Fujii Hironori]

Comment on attachment 375147 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:639
>> +void WebPageProxy::setPageLoadStateObserver(UniqueRef<PageLoadState::Observer>&& observer)
> 
> My earlier feedback was mistaken.  We ought to be able to call WKPageSetPageStateClient with nullptr and have it clear the observer.  That would bring this back to a std::unique_ptr.  Please do that, add that to the test, and r=me.

WKPageSetPageStateClient can accept nullptr without modification for this patch. 
However, I get the idea. I'll change WebPageProxy::setPageLoadStateObserver to take a std::unique_ptr.

[Fujii Hironori]

Created attachment 375203 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 375203 [details]
Patch for landing

Clearing flags on attachment: 375203

Committed r248029: <https://trac.webkit.org/changeset/248029>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53745381>

[Fujii Hironori]

Filed: Bug 200465 – [WebKit] ASSERTION FAILED: m_observers.isEmpty() if WKPageSetPageStateClient is used