Summary: | [WebKit] Add PageLoadState::Observer C API | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Fujii Hironori
2019-07-16 20:52:10 PDT
Created attachment 374275 [details]
Patch
Created attachment 374280 [details]
Patch
Comment on attachment 374280 [details]
Patch
There's nothing windows specific about this. Why not make it cross-platform?
Agreed. Will do so. Created attachment 374591 [details]
Patch
Created attachment 374755 [details]
Patch
Created attachment 374758 [details]
Patch
r? 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. Created attachment 375146 [details]
Patch
* Addressed review feedbacks
Created attachment 375147 [details]
Patch
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 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. Created attachment 375203 [details]
Patch for landing
Comment on attachment 375203 [details] Patch for landing Clearing flags on attachment: 375203 Committed r248029: <https://trac.webkit.org/changeset/248029> All reviewed patches have been landed. Closing bug. Filed: Bug 200465 – [WebKit] ASSERTION FAILED: m_observers.isEmpty() if WKPageSetPageStateClient is used |