[webkit-reviews] review denied: [Bug 33812] Need way to be notified when a page changes its favicon : [Attachment 47351] Should fix qt build failure

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Dave Moore
<davemoore at google.com>'s request for review:
Bug 33812: Need way to be notified when a page changes its favicon
https://bugs.webkit.org/show_bug.cgi?id=33812

Attachment 47351: Should fix qt build failure
https://bugs.webkit.org/attachment.cgi?id=47351&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Lots of comments in the first pass. Welcome to WebKit-land :)

> +    if (Frame* f = frame())
> +	   f->loader()->setIconURL(m_iconURL);

In WebKit, we try to use descriptive variable names: Frame* frame =
this->frame()

> +    if (loader == m_documentLoader) {
> +	   m_client->dispatchDidReceiveIconURL(loader->iconURL());
> +	   // TODO[davemoore at chromium.org] How do we update the icons for
history?

TODO[...] => FIXME:


> +void FrameLoaderClientImpl::willChangeIconURL(DocumentLoader*)
> +{
> +    // FIXME

A bit more comment here?

> +}
> +
> +void FrameLoaderClientImpl::didChangeIconURL(DocumentLoader*)
> +{
> +    // FIXME
> +}

Ditto.

> Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
> ===================================================================
> --- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(revision
53780)
> +++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(working copy)
> @@ -749,6 +749,11 @@ void FrameLoaderClient::dispatchDidRecei
>      }
>  }
>  
> +void FrameLoaderClient::dispatchDidReceiveIconURL(const String& iconURL)
> +{
> +    notImplemented();

Why is it notImplemented() here and FIXME in other ports?

>  
> +void FrameLoaderClient::willChangeIconURL(WebCore::DocumentLoader*)
> +{
> +    notImplemented();

Ditto.

> +void FrameLoaderClient::didChangeIconURL(WebCore::DocumentLoader*)
> +{
> +    // FIXME implement to update favicon

Colon after FIXME and make it a sentence.

FIXME: Implement to update favicon.

> +void WebFrameLoaderClient::dispatchDidReceiveIconURL(const String& iconURL)

I think you should omit parameter name here -- otherwise, compiler will yell at
you.

> +void WebFrameLoaderClient::willChangeIconURL(DocumentLoader* loader)

Ditto.

> +{
> +    // FIXME: Implement to send notification if icon url is about to change
> +}
> +
> +void WebFrameLoaderClient::didChangeIconURL(DocumentLoader* loader)

Ditto.

>  
>      /*!
> +	   @method webView:didReceiveIconURL:forFrame:
> +	   @abstract Notifies the delegate that the page icon URL for a frame
has been received
> +	   @param webView The WebView sending the message
> +	   @param iconURL The new icon url
> +	   @param frame The frame for which the icon URL has been received
> +	   - (void)webView:(WebView *)sender didReceiveIconURL:(NSString
*)iconURL forFrame:(WebFrame *)frame;
> +    */
> +    HRESULT didReceiveIconURL([in] IWebView* webView, [in] BSTR iconURL,
[in] IWebFrame* frame);
> +
> +    /*!

I have no clue about this parts of WebKit. Need aroben to look.


Also, you should modify platform/(mac|qt|gtk)/Skipped, if you're not planning
to implement DRT changes for them.