Bug 199848

Summary: [WebKit] Add PageLoadState::Observer C API
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: WebKit2Assignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, cdumez, commit-queue, don.olmstead, rniwa, ross.kirsling, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189336
https://bugs.webkit.org/show_bug.cgi?id=200465
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Fujii Hironori 2019-07-16 20:52:10 PDT
[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.
Comment 1 Fujii Hironori 2019-07-16 20:58:26 PDT
Created attachment 374275 [details]
Patch
Comment 2 Fujii Hironori 2019-07-16 21:21:45 PDT
Created attachment 374280 [details]
Patch
Comment 3 Alex Christensen 2019-07-19 10:46:36 PDT
Comment on attachment 374280 [details]
Patch

There's nothing windows specific about this.  Why not make it cross-platform?
Comment 4 Fujii Hironori 2019-07-21 18:42:45 PDT
Agreed. Will do so.
Comment 5 Fujii Hironori 2019-07-22 03:09:55 PDT
Created attachment 374591 [details]
Patch
Comment 6 Fujii Hironori 2019-07-23 20:22:09 PDT
Created attachment 374755 [details]
Patch
Comment 7 Fujii Hironori 2019-07-23 20:51:19 PDT
Created attachment 374758 [details]
Patch
Comment 8 Fujii Hironori 2019-07-26 00:54:25 PDT
r?
Comment 9 Alex Christensen 2019-07-29 10:11:39 PDT
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.
Comment 10 Fujii Hironori 2019-07-29 23:00:38 PDT
Created attachment 375146 [details]
Patch

* Addressed review feedbacks
Comment 11 Fujii Hironori 2019-07-30 00:23:04 PDT
Created attachment 375147 [details]
Patch
Comment 12 Alex Christensen 2019-07-30 10:18:11 PDT
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.
Comment 13 Fujii Hironori 2019-07-30 19:11:24 PDT
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.
Comment 14 Fujii Hironori 2019-07-30 19:36:59 PDT
Created attachment 375203 [details]
Patch for landing
Comment 15 Fujii Hironori 2019-07-30 22:14:59 PDT
Comment on attachment 375203 [details]
Patch for landing

Clearing flags on attachment: 375203

Committed r248029: <https://trac.webkit.org/changeset/248029>
Comment 16 Fujii Hironori 2019-07-30 22:15:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-07-30 22:16:18 PDT
<rdar://problem/53745381>
Comment 18 Fujii Hironori 2019-08-05 23:44:11 PDT
Filed: Bug 200465 – [WebKit] ASSERTION FAILED: m_observers.isEmpty() if WKPageSetPageStateClient is used