Bug 172513 – REGRESSION(r217051): Automation sessions fail to complete bootstrap

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

We got too strict.

[Blaze Burg]

<rdar://problem/32338354>

[Blaze Burg]

Created attachment 311046 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311046 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        WIRAutomatically is an optional key in the setup message. In the relay, this key gets copied
> +        into an NSDictionary as NSNull if the key isn't present in a forwarded command.

I don't see how this can happen. It could be optional but as it exists right now I don't think it is.

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:58
> +#define BAIL_IF_UNEXPECTED_TYPE_ALLOWING_NIL(expr, classExpr)   \
> +    do {                                                        \
> +        id value = (expr);                                      \
> +        if ([value isEqualTo:[NSNull null]])                    \
> +            expr = value = nil;                                 \

This should be named differently since we have the ALLOWING_NIL name in WebInspector which doesn't check for NSNull. Also its hiding an operation that I don't think should be hidden. I'd rather see two operations:

    CONVERT_NSNULL_TO_NIL(automaticallyPauseNumber);
    BAIL_IF_UNEXPECTED_TYPE_ALLOWING_NIL(...);

That said, I still don't believe it should even be NSNull.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 311046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311046&action=review
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        WIRAutomatically is an optional key in the setup message. In the relay, this key gets copied
> > +        into an NSDictionary as NSNull if the key isn't present in a forwarded command.
> 
> I don't see how this can happen. It could be optional but as it exists right
> now I don't think it is.

Oh. Apparently this is possible only for Web Driver. Thats unfortunate.


> > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:58
> > +#define BAIL_IF_UNEXPECTED_TYPE_ALLOWING_NIL(expr, classExpr)   \
> > +    do {                                                        \
> > +        id value = (expr);                                      \
> > +        if ([value isEqualTo:[NSNull null]])                    \
> > +            expr = value = nil;                                 \
> 
> This should be named differently since we have the ALLOWING_NIL name in
> WebInspector which doesn't check for NSNull. Also its hiding an operation
> that I don't think should be hidden. I'd rather see two operations:
> 
>     CONVERT_NSNULL_TO_NIL(automaticallyPauseNumber);
>     BAIL_IF_UNEXPECTED_TYPE_ALLOWING_NIL(...);
> 
> That said, I still don't believe it should even be NSNull.

Splitting this up into two operations would be much clearer.

r=me with that change

[Blaze Burg]

Created attachment 311053 [details]
For landing

[Blaze Burg]

Created attachment 311067 [details]
For landing

[Build Bot]

Comment on attachment 311067 [details]
For landing

Attachment 311067 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3802999

New failing tests:
fetch/closing-while-fetching-blob.html
fast/css/target-fragment-match.html

[Build Bot]

Created attachment 311074 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Blaze Burg]

Committed r217309: <http://trac.webkit.org/changeset/217309>