Bug 207185 - _WKDownload should expose the originating FrameInfo
Summary: _WKDownload should expose the originating FrameInfo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-03 22:25 PST by Brady Eidson
Modified: 2020-02-05 13:05 PST (History)
5 users (show)

See Also:

Attachments
Patch (16.30 KB, patch)
2020-02-03 22:44 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (21.24 KB, patch)
2020-02-04 16:15 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (22.53 KB, patch)
2020-02-04 16:54 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (22.16 KB, patch)
2020-02-05 12:46 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2020-02-05 12:49 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2020-02-03 22:25:33 PST 
_WKDownload should expose the originating FrameInfo

<rdar://problem/58022576>
Comment 1 Brady Eidson 2020-02-03 22:44:22 PST 
Created attachment 389633 [details]
Patch
Comment 2 Brady Eidson 2020-02-03 22:44:55 PST 
API test to come, but just wanted EWS to check the build, existing API tests, etc.
Comment 3 Brady Eidson 2020-02-04 16:15:12 PST 
Created attachment 389731 [details]
Patch
Comment 4 Geoffrey Garen 2020-02-04 16:22:56 PST 
Comment on attachment 389731 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:91
> +    if (frameInfo.page() == nullptr)
> +        frameInfo.setPage(_download->originatingPage());

Under what conditions will a FrameInfo lack a page, and why is it correct under those conditions to claim that its page was the page that originated the download? (Does the API test demonstrate this case?)
Comment 5 Brady Eidson 2020-02-04 16:37:32 PST 
(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 389731 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389731&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:91
> > +    if (frameInfo.page() == nullptr)
> > +        frameInfo.setPage(_download->originatingPage());
> 
> Under what conditions will a FrameInfo lack a page,

Most callers of DownloadProxy::create don't have a page available, so they are never created with a page, so the answer is "always"

> Does the API test demonstrate this case?

It does.

That said, I'll just go rework the DownloadProxy c'tor some more.
Comment 6 Brady Eidson 2020-02-04 16:54:32 PST 
Created attachment 389745 [details]
Patch
Comment 7 Geoffrey Garen 2020-02-05 11:15:18 PST 
Comment on attachment 389745 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/APIFrameInfo.h:57
> +    void setPage(WebKit::WebPageProxy* page) { m_page = page; }

You can remove this setter now.

> Source/WebKit/UIProcess/API/Cocoa/_WKDownload.h:45
> +@property (nonatomic, readonly) WKFrameInfo *originatingFrameInfo WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Let's call this "originatingFrame". Even though the data type is WKFrameInfo, I think "frame" reads better, and it appears to be more consistent with our other WKFrameInfo APIs.
Comment 8 Brady Eidson 2020-02-05 12:46:24 PST 
Created attachment 389849 [details]
Patch
Comment 9 Brady Eidson 2020-02-05 12:49:43 PST 
Created attachment 389851 [details]
Patch
Comment 10 Alex Christensen 2020-02-05 13:05:47 PST 
http://trac.webkit.org/r255845