Summary: | [WinCairo][PlayStation] Add automation support for RemoteInspector SocketServer implementation. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||
Component: | Platform | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | Basuke.Suzuki, cgarcia, chris.reid, commit-queue, ews-watchlist, hi, joepeck, keith_miller, mark.lam, mcatanzaro, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Basuke Suzuki
2019-06-20 10:35:15 PDT
Created attachment 380774 [details]
PATCH
Created attachment 380775 [details]
PATCH
Basically following GTK implementation. Comment on attachment 380775 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=380775&action=review > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:200 > + if (!m_client) > + return; > + > + if (!m_clientCapabilities || !m_clientCapabilities->remoteAutomationAllowed) > + return; > + > + if (sessionID.isNull()) > + return; Not sure that we need three separate conditionals here. > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:359 > + String sessionID = *event.message; > + requestAutomationSession(WTFMove(sessionID), { }); This style is a bit confusing. At the least, I think `event.message.value()` would be more readable, but I also wonder if it's worth making the new requestAutomationSession take const String& instead, so that we wouldn't have to explicitly copy. Comment on attachment 380775 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=380775&action=review >> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:200 >> + return; > > Not sure that we need three separate conditionals here. Latter two are runtime check which caused by configuration so I'll add different LOG_ERROR to them. First one is the correct check which caused by timing and required check. >> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:359 >> + requestAutomationSession(WTFMove(sessionID), { }); > > This style is a bit confusing. At the least, I think `event.message.value()` would be more readable, but I also wonder if it's worth making the new requestAutomationSession take const String& instead, so that we wouldn't have to explicitly copy. Right. I'll change this to const String& because last step of this function calls may ended up with const String& and using `move` here is meaningless. Created attachment 381013 [details]
PATCH
Comment on attachment 381013 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=381013&action=review > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:152 > + void requestAutomationSession(const String& sessionID, const Client::SessionCapabilities&); Final nit: I guess the string name can be omitted here. Comment on attachment 381013 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=381013&action=review Thanks! >> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:152 >> + void requestAutomationSession(const String& sessionID, const Client::SessionCapabilities&); > > Final nit: I guess the string name can be omitted here. No I don't think so. General types such as bool, int or String should have its name to make it meaningful. Comment on attachment 381013 [details] PATCH Clearing flags on attachment: 381013 Committed r251373: <https://trac.webkit.org/changeset/251373> All reviewed patches have been landed. Closing bug. |