Bug 204338 – [WinCairo] Refine initialization and error handling in RemoteInspectorSocket

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Yousuke Kimoto]

RemoteInspectorSocket should handle some error cases to avoid ambiguous conditions.

[Yousuke Kimoto]

Created attachment 383849 [details]
patch

[Fujii Hironori]

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.

[Yousuke Kimoto]

(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.)

[Yousuke Kimoto]

Created attachment 384998 [details]
patch

[Yousuke Kimoto]

Thank you for your review.

[WebKit Commit Bot]

Comment on attachment 384998 [details]
patch

Clearing flags on attachment: 384998

Committed r253281: <https://trac.webkit.org/changeset/253281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57737371>