Summary: | Send TestRendered event after running a test but before dumping | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||
Component: | Tools / Tests | Assignee: | Darin Adler <darin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | annulen, ap, ews-watchlist, gyuyoung.kim, Hironori.Fujii, ryuan.choi, sam, sergio, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=186045 https://bugs.webkit.org/show_bug.cgi?id=216397 |
||||||||||||
Bug Depends on: | 216440 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Darin Adler
2020-09-11 18:05:56 PDT
Created attachment 408584 [details]
Patch
This sends the event. Might need to refine this later to make sure we force rendering twice the way the WPT documentation suggests; doing that unconditionally would probably slow down test running. I was about to say that there were iOS expectations to remove too, but there aren't. Not sure what happened. Comment on attachment 408584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408584&action=review > Tools/TestRunnerShared/Bindings/JSBasics.h:38 > +JSValueRef JSValueMakeBooleanOrNull(JSContextRef, Optional<bool>); > +Optional<bool> JSValueToNullableBoolean(JSContextRef, JSValueRef); > + > +JSValueRef JSValueMakeStringOrNull(JSContextRef, JSStringRef); Since you are just moving this, we shouldn't change this for this change, but I don't think we should mimic the JavaScriptCore c-API naming style here. Should instead be something like: JSValueMakeBooleanOrNull(JSContextRef, Optional<bool>) -> makeValue(JSContextRef, Optional<bool>) JSValueToNullableBoolean(JSContextRef, JSValueRef) -> toOptionalBool(JSContextRef, JSValueRef) JSValueMakeStringOrNull(JSContextRef, JSStringRef) -> makeValueAllowingNull(JSContextRef, JSStringRef) (In reply to Sam Weinig from comment #4) > Since you are just moving this, we shouldn't change this for this change, > but I don't think we should mimic the JavaScriptCore c-API naming style > here. Should instead be something like: > > JSValueMakeBooleanOrNull(JSContextRef, Optional<bool>) -> > makeValue(JSContextRef, Optional<bool>) > JSValueToNullableBoolean(JSContextRef, JSValueRef) -> > toOptionalBool(JSContextRef, JSValueRef) > JSValueMakeStringOrNull(JSContextRef, JSStringRef) -> > makeValueAllowingNull(JSContextRef, JSStringRef) Yes, I absolutely agree. You will see I chose names like that for the newer functions I created recently and even renamed one of the old ones, but not all of them yet. Is there anyone with Windows and CMake build expertise that can help me figure out why the WinCairo build is failing? Something about the same precompiled header being used with different /D options? (In reply to Sam Weinig from comment #4) > JSValueMakeBooleanOrNull(JSContextRef, Optional<bool>) -> > makeValue(JSContextRef, Optional<bool>) Yes. > JSValueToNullableBoolean(JSContextRef, JSValueRef) -> > toOptionalBool(JSContextRef, JSValueRef) Maybe optionalBoolean or optionalBooleanValue instead? Never sure whether to name after the C++ type keyword or after the full name of the type. Also not sure we need the "to" in a case like this. > JSValueMakeStringOrNull(JSContextRef, JSStringRef) -> > makeValueAllowingNull(JSContextRef, JSStringRef) I think just makeValue. If we decide we need one that turns nullptr into an assertion, a crash, or an empty string we can give it the more unusual name. Can’t land this until: 1) the fix for bug 216440 gets landed first, since I’ll have to merge with it 2) I figure out what to do on WinCairo. I do not understand why the build there is failing, since I put JSBasics.cpp in all the same places where JSWrapper.cpp was already mentioned. This could delay me indefinitely! Created attachment 408630 [details]
Patch to disable the precompiled header for WinCairo DRT and WTR
Please disable the precompiled header for WinCairo DRT and WTR at the moment. I'll look into it next Monday.
Created attachment 408641 [details]
Patch
Created attachment 408642 [details]
Patch
Committed r266988: <https://trac.webkit.org/changeset/266988> |