Bug 199848 - [WebKit] Add PageLoadState::Observer C API
Summary: [WebKit] Add PageLoadState::Observer C API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-16 20:52 PDT by Fujii Hironori
Modified: 2019-08-05 23:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (23.62 KB, patch)
2019-07-16 20:58 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (30.69 KB, patch)
2019-07-16 21:21 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (27.69 KB, patch)
2019-07-22 03:09 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (31.94 KB, patch)
2019-07-23 20:22 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (35.02 KB, patch)
2019-07-23 20:51 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (44.65 KB, patch)
2019-07-29 23:00 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (44.55 KB, patch)
2019-07-30 00:23 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (45.06 KB, patch)
2019-07-30 19:36 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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