RemoteInspectorSocket should handle some error cases to avoid ambiguous conditions.
Created attachment 383849 [details] patch
Comment on attachment 383849 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=383849&action=review > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:221 > + LOG_ERROR("createPair() error at getting server port (errno = %d)", WSAGetLastError()); You don't need to include the function name "createPair()" in LOG_ERROR message because it contains. #define LOG_ERROR(...) WTFReportError(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, __VA_ARGS__) I think this LOG_ERROR is unnecessary because getPort prints log if it'd fail. > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:224 > + address.sin_port = htons((u_short)*serverPort); Why did you add this cast (u_short)? I think it's unnecessary. And, WebKit is merely using C style cast. Use static_cast<u_short>() if needed.
(In reply to Fujii Hironori from comment #2) > > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:221 > > + LOG_ERROR("createPair() error at getting server port (errno = %d)", WSAGetLastError()); > > You don't need to include the function name "createPair()" in LOG_ERROR > message because it contains. I understand, and delete the redundant error message. > #define LOG_ERROR(...) WTFReportError(__FILE__, __LINE__, > WTF_PRETTY_FUNCTION, __VA_ARGS__) > > I think this LOG_ERROR is unnecessary because getPort prints log if it'd > fail. Actually getPort() in RemoteInspectorSocketWin.cpp doesn't print an error message though (getPort() in RemoteInspectorSocketPOSIX.cpp has it), it is a good style that the function prints the message. > > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:224 > > + address.sin_port = htons((u_short)*serverPort); > > Why did you add this cast (u_short)? I think it's unnecessary. And, WebKit > is merely using C style cast. Use static_cast<u_short>() if needed. Right, it's unnecessary. (I saw a cast warning at the line, but it seemed I was confused.)
Created attachment 384998 [details] patch
Thank you for your review.
Comment on attachment 384998 [details] patch Clearing flags on attachment: 384998 Committed r253281: <https://trac.webkit.org/changeset/253281>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57737371>