| Summary: | [Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and modern WebKit | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||
| Component: | WebKit API | Assignee: | Darin Adler <darin> | ||||
| Status: | ASSIGNED --- | ||||||
| Severity: | Normal | CC: | cdumez, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, philipj, rniwa, sergio, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Darin Adler
2021-03-15 16:25:48 PDT
Created attachment 423263 [details]
Patch
Comment on attachment 423263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423263&action=review Looks good to me. > Source/WebCore/platform/mac/ThreadCheck.mm:74 > + NSLog(@"WebKit Threading Violation - %s was called from secondary thread", function); Maybe we should be using RELEASE_LOG_ERROR() so that its stands out in console logging as an error? > Source/WebCore/platform/mac/ThreadCheck.mm:75 > + NSLog(@"Additional threading violations for this function will not be logged."); ditto. > Source/WebCore/platform/mac/ThreadCheck.mm:78 > + NSLog(@"WebKit Threading Violation - %s was called from secondary thread", function); Ditto. Comment on attachment 423263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423263&action=review > Source/WebCore/platform/ThreadCheck.h:40 > + using BehaviorArray = std::array<Behavior, std::numeric_limits<uint8_t>::max()>; It might be better to have a single global round / behavior versions so that there is no confusion about "which round one" we're talking about. > Source/WebCore/platform/ThreadCheck.h:53 > + static std::once_flag flag; > + std::call_once(flag, [] { > + initializeBehaviors(setDefaultBehaviors); > + }); Hm... I think it would be more clear to have this code in the constructor of ThreadViolationChecker. I wonder if we can write API tests for these... Comment on attachment 423263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423263&action=review >> Source/WebCore/platform/ThreadCheck.h:53 >> + }); > > Hm... I think it would be more clear to have this code in the constructor of ThreadViolationChecker. The reason it’s here is so that the caller can pass in the function as a template argument. I didn’t want the class itself to be a class template. Comment on attachment 423263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423263&action=review >> Source/WebCore/platform/ThreadCheck.h:40 >> + using BehaviorArray = std::array<Behavior, std::numeric_limits<uint8_t>::max()>; > > It might be better to have a single global round / behavior versions > so that there is no confusion about "which round one" we're talking about. The design here is that WebKit has a set of rounds, and WebKitLegacy has a separate set. That’s why each object has a different behavior array. One for WebKit and one for WebKitLegacy. I thought it would be clearer to keep them separate, since they are based on different linked-on-or-after checks. Could go another way, of course. |