Bug 192473

Summary: WKWebProcessPlugInLoadDelegate should have API for saying which rendering events it wants to listen for
Product: WebKit Reporter: Saam Barati <sbarati>
Component: WebKit2Assignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, beidson, cdumez, commit-queue, ggaren, koivisto, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
aestes: review+
patch for landing none

Description Saam Barati 2018-12-06 13:08:32 PST
...
Comment 1 Saam Barati 2018-12-06 13:08:48 PST
(In reply to Saam Barati from comment #0)
> ...

Patch forthcoming
Comment 2 Saam Barati 2018-12-06 16:48:02 PST
Created attachment 356765 [details]
patch
Comment 3 Andy Estes 2018-12-06 16:53:24 PST
Comment on attachment 356765 [details]
patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
> +- (_WKRenderingProgressEvents)getLayoutMilestonesWithWebProcessPlugInBrowserContextController:(WKWebProcessPlugInBrowserContextController*)controller;

The layout milestones / rendering progress events terminology mismatch is a little strange here.

But assuming you keep "layout milestones" term, the right name for this selector would be -webProcessPlugInBrowserContextControllerLayoutMilestones:.
Comment 4 Saam Barati 2018-12-06 17:28:57 PST
(In reply to Andy Estes from comment #3)
> Comment on attachment 356765 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356765&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
> > +- (_WKRenderingProgressEvents)getLayoutMilestonesWithWebProcessPlugInBrowserContextController:(WKWebProcessPlugInBrowserContextController*)controller;
> 
> The layout milestones / rendering progress events terminology mismatch is a
> little strange here.
Good point. I forgot to change the name to reflect the type when I changed the type. Will update.
> 
> But assuming you keep "layout milestones" term, the right name for this
> selector would be -webProcessPlugInBrowserContextControllerLayoutMilestones:.

👍🏼
Comment 5 Saam Barati 2018-12-06 17:32:22 PST
Created attachment 356771 [details]
patch
Comment 6 Andy Estes 2018-12-07 11:33:35 PST
Comment on attachment 356771 [details]
patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
> +- (_WKRenderingProgressEvents)webProcessPlugInBrowserContextControllerRenderingProgressEvents:(WKWebProcessPlugInBrowserContextController*)controller;

WKWebProcessPlugInBrowserContextController *
Comment 7 Saam Barati 2018-12-07 12:39:54 PST
Comment on attachment 356771 [details]
patch

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

>> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
>> +- (_WKRenderingProgressEvents)webProcessPlugInBrowserContextControllerRenderingProgressEvents:(WKWebProcessPlugInBrowserContextController*)controller;
> 
> WKWebProcessPlugInBrowserContextController *

Will fix. Thanks for the review.
Comment 8 Saam Barati 2018-12-07 12:41:40 PST
Created attachment 356831 [details]
patch for landing
Comment 9 WebKit Commit Bot 2018-12-07 14:35:14 PST
Comment on attachment 356831 [details]
patch for landing

Clearing flags on attachment: 356831

Committed r238969: <https://trac.webkit.org/changeset/238969>
Comment 10 WebKit Commit Bot 2018-12-07 14:35:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-12-07 14:36:28 PST
<rdar://problem/46565296>
Comment 12 Alex Christensen 2018-12-07 17:01:58 PST
Comment on attachment 356831 [details]
patch for landing

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

> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
> +- (_WKRenderingProgressEvents)webProcessPlugInBrowserContextControllerRenderingProgressEvents:(WKWebProcessPlugInBrowserContextController *)controller;

I think this should've been completion handler based.  There's no reason this needs to be synchronous.
Comment 13 Saam Barati 2018-12-08 09:38:20 PST
(In reply to Alex Christensen from comment #12)
> Comment on attachment 356831 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356831&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h:56
> > +- (_WKRenderingProgressEvents)webProcessPlugInBrowserContextControllerRenderingProgressEvents:(WKWebProcessPlugInBrowserContextController *)controller;
> 
> I think this should've been completion handler based.  There's no reason
> this needs to be synchronous.

That’s not right. WebKit calls this expecting a response