Bug 216428 – Send TestRendered event after running a test but before dumping

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Darin Adler]

https://web-platform-tests.org/writing-tests/reftests.html

[Darin Adler]

Created attachment 408584 [details]
Patch

[Darin Adler]

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.

[Alexey Proskuryakov]

I was about to say that there were iOS expectations to remove too, but there aren't. Not sure what happened.

[Sam Weinig]

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)

[Darin Adler]

(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.

[Darin Adler]

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?

[Darin Adler]

(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.

[Darin Adler]

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!

[Fujii Hironori]

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.

[Darin Adler]

Created attachment 408641 [details]
Patch

[Darin Adler]

Created attachment 408642 [details]
Patch

[Darin Adler]

Committed r266988: <https://trac.webkit.org/changeset/266988>

[Radar WebKit Bug Importer]

<rdar://problem/68790483>