[webkit-reviews] review granted: [Bug 45682] [WINCE] Add FrameLoaderClientWinCE : [Attachment 67426] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 45682: [WINCE] Add FrameLoaderClientWinCE
https://bugs.webkit.org/show_bug.cgi?id=45682

Attachment 67426: Patch
https://bugs.webkit.org/attachment.cgi?id=67426&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=67426&action=review

> WebKit/wince/ChangeLog:9
> +	   * WebCoreSupport/FrameLoaderClientWinCE.cpp: Added.
> +	   (WebKit::FrameLoaderClient::FrameLoaderClient):

WebKit's standard style is for the file name to match the class name (ignoring
namespaces). So the file name should be FrameLoaderClient.cpp. I see that the
other files in this directory use the pattern you've used here, though.

> WebKit/wince/ChangeLog:28
> +	   (WebKit::FrameLoaderClient::~FrameLoaderClient):
> +	   (WebKit::FrameLoaderClient::userAgent):
> +	   (WebKit::FrameLoaderClient::createDocumentLoader):
> +	   (WebKit::FrameLoaderClient::committedLoad):
> +	   (WebKit::FrameLoaderClient::shouldUseCredentialStorage):
> +	  
(WebKit::FrameLoaderClient::dispatchDidReceiveAuthenticationChallenge):
> +	  
(WebKit::FrameLoaderClient::dispatchDidCancelAuthenticationChallenge):
> +	   (WebKit::FrameLoaderClient::dispatchWillSendRequest):
> +	   (WebKit::FrameLoaderClient::assignIdentifierToInitialRequest):
> +	   (WebKit::FrameLoaderClient::postProgressStartedNotification):
> +	  
(WebKit::FrameLoaderClient::postProgressEstimateChangedNotification):
> +	   (WebKit::FrameLoaderClient::postProgressFinishedNotification):
> +	   (WebKit::FrameLoaderClient::frameLoaderDestroyed):
> +	   (WebKit::FrameLoaderClient::dispatchDidReceiveResponse):
> +	   (WebKit::FrameLoaderClient::dispatchDecidePolicyForMIMEType):
> +	   (WebKit::FrameLoaderClient::dispatchDecidePolicyForNewWindowAction):

> +	  
(WebKit::FrameLoaderClient::dispatchDecidePolicyForNavigationAction):
> +	   (WebKit::FrameLoaderClient::dispatchWillSubmitForm):
> +	   (WebKit::FrameLoaderClient::createPlugin):

All these uncommented-on function names don't really make the ChangeLog any
more helpful. You could group them into categories (implemented vs.
unimplemented and needs implementation vs. unimplemented and doesn't need
implementation). Or you could omit all but the functions you've implemented
(and maybe comment on those). Or you could omit all of them.

> WebKit/wince/WebCoreSupport/FrameLoaderClientWinCE.cpp:59
> +WTF::PassRefPtr<WebCore::DocumentLoader>
FrameLoaderClient::createDocumentLoader(const WebCore::ResourceRequest&
request, const SubstituteData& substituteData)

No need for WTF:: here.