Bug 214947 - REGRESSION (r264925): run-safari --debug no longer works
Summary: REGRESSION (r264925): run-safari --debug no longer works
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-29 16:11 PDT by Simon Fraser (smfr)
Modified: 2020-07-30 09:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (19.24 KB, patch)
2020-07-29 17:53 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (19.31 KB, patch)
2020-07-30 09:03 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-07-29 16:11:30 PDT
$ ./Tools/Scripts/run-safari --debug
Starting SafariForWebKitDevelopment with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Users/kbr/src/WebKit/WebKitBuild/Debug.
dyld: Symbol not found: _WKBundlePageCopyRenderLayerTree
  Referenced from: /System/Library/PrivateFrameworks/Safari.framework/Versions/A/Safari
  Expected in: /Users/kbr/src/WebKit/WebKitBuild/Debug/WebKit.framework/Versions/A/WebKit
 in /System/Library/PrivateFrameworks/Safari.framework/Versions/A/Safari
Comment 1 Radar WebKit Bug Importer 2020-07-29 16:14:04 PDT
<rdar://problem/66297850>
Comment 2 Kate Cheney 2020-07-29 17:53:41 PDT
Created attachment 405535 [details]
Patch
Comment 3 Darin Adler 2020-07-29 18:04:00 PDT
Comment on attachment 405535 [details]
Patch

This is fine:

- If these functions are linked but not called, you might want to put ASSERT_NOT_REACHED in them to help people understand they are intentionally left blank, or a comment, or both. I did that recently with some SPI that I had to leave around for the same reason.

- It’s also nice to leave out the argument names; if we complied with "unused argument" warnings, that would prevent them.

- Also, sometimes we move these things out of the SPI headers into an "old Safari depends on these" block, often not even in a header file, to help make sure no one accidentally adds new calls to the functions.

Not saying you need to do any of these things. Seems like you could land this just like it is.
Comment 4 Kate Cheney 2020-07-30 09:03:17 PDT
Created attachment 405575 [details]
Patch for landing
Comment 5 Kate Cheney 2020-07-30 09:08:04 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 405535 [details]
> Patch
> 

Thanks for the review, and the suggestions.

> This is fine:
> 
> - If these functions are linked but not called, you might want to put
> ASSERT_NOT_REACHED in them to help people understand they are intentionally
> left blank, or a comment, or both. I did that recently with some SPI that I
> had to leave around for the same reason.
> 

I added comments to explain why the functions were intentionally left blank. Adding ASSERT_NOT_REACHED() caused the compiler to warn that the function is "noreturn".
Comment 6 EWS 2020-07-30 09:31:45 PDT
Committed r265085: <https://trac.webkit.org/changeset/265085>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405575 [details].