Bug 174659

Summary: [WebIDL] Remove custom bindings for InspectorFrontendHost
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, joepeck
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2 none

Description Sam Weinig 2017-07-19 12:33:54 PDT
[WebIDL] Remove custom bindings for InspectorFrontendHost
Comment 1 Sam Weinig 2017-07-19 12:45:18 PDT
Created attachment 315949 [details]
Patch
Comment 2 Build Bot 2017-07-19 14:11:26 PDT
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
Comment 3 Build Bot 2017-07-19 14:11:27 PDT
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
Comment 4 Sam Weinig 2017-07-19 21:56:36 PDT
I do not believe the test failure is related.
Comment 5 Chris Dumez 2017-07-20 08:50:08 PDT
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?
Comment 6 Sam Weinig 2017-07-20 09:29:41 PDT
(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.
Comment 7 WebKit Commit Bot 2017-07-20 09:58:09 PDT
Comment on attachment 315949 [details]
Patch

Clearing flags on attachment: 315949

Committed r219691: <http://trac.webkit.org/changeset/219691>
Comment 8 WebKit Commit Bot 2017-07-20 09:58:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2017-07-20 10:14:07 PDT
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?
Comment 10 Joseph Pecoraro 2017-07-20 14:29:32 PDT
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.
Comment 11 Chris Dumez 2017-07-20 14:32:19 PDT
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
Comment 12 Sam Weinig 2017-07-21 08:52:51 PDT
(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.