Bug 174659 - [WebIDL] Remove custom bindings for InspectorFrontendHost
Summary: [WebIDL] Remove custom bindings for InspectorFrontendHost
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-19 12:33 PDT by Sam Weinig
Modified: 2017-07-21 08:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.41 KB, patch)
2017-07-19 12:45 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (968.34 KB, application/zip)
2017-07-19 14:11 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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.