Bug 174659 – [WebIDL] Remove custom bindings for InspectorFrontendHost

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Remove custom bindings for InspectorFrontendHost

[Sam Weinig]

Created attachment 315949 [details]
Patch

[Build Bot]

Comment on attachment 315949 [details]
Patch

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

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open.html

[Build Bot]

Created attachment 315956 [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.12.5

[Sam Weinig]

I do not believe the test failure is related.

[Chris Dumez]

Comment on attachment 315949 [details]
Patch

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

r=me

> Source/WebCore/inspector/InspectorFrontendHost.idl:82
> +    JSGenerateToJSObject

Is this really needed?

[Sam Weinig]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 315949 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315949&action=review
> 
> r=me
> 
> > Source/WebCore/inspector/InspectorFrontendHost.idl:82
> > +    JSGenerateToJSObject
> 
> Is this really needed?

Yep.

[WebKit Commit Bot]

Comment on attachment 315949 [details]
Patch

Clearing flags on attachment: 315949

Committed r219691: <http://trac.webkit.org/changeset/219691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 315949 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorFrontendHost.idl:82
>>> +    JSGenerateToJSObject
>> 
>> Is this really needed?
> 
> Yep.

Ok but why is that? :) We never return this type to JS, do we? It only seems to be used for an input parameter. So if we don't return this type to JS, why do we need a toJS function for it?

[Joseph Pecoraro]

Comment on attachment 315949 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendHost.idl:71
> +    void showContextMenu(Event? event, sequence<ContextMenuItem> items);

`event` here can be made non-optional. All uses in Web Inspector pass a non-null value and would expect to be providing a valid Event.

[Chris Dumez]

Comment on attachment 315949 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFrontendHost.idl:71
>> +    void showContextMenu(Event? event, sequence<ContextMenuItem> items);
> 
> `event` here can be made non-optional. All uses in Web Inspector pass a non-null value and would expect to be providing a valid Event.

it is not currently optional, it is nullable :P

[Sam Weinig]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 315949 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315949&action=review
> 
> >>> Source/WebCore/inspector/InspectorFrontendHost.idl:82
> >>> +    JSGenerateToJSObject
> >> 
> >> Is this really needed?
> > 
> > Yep.
> 
> Ok but why is that? :) We never return this type to JS, do we? It only seems
> to be used for an input parameter. So if we don't return this type to JS,
> why do we need a toJS function for it?

I don't know what I was thinking. You are right, it is not needed. I could have sworn I added it for a reason, but guess not. Fixed in r219726.