Bug 20306 – [gtk] implements FrameLoaderClient::dispatchDidChangeLocationWithinPage

• Component: WebKitGTK
• Status: RESOLVED FIXED

[arno.]

Hi,
When a user clicks an internal link on a webpage (a link to an anchor within that page), document location changes, but webkit-gtk embedders are not notified of that change. 

Therefore, it would be nice to implement FrameLoaderClient::dispatchDidChangeLocationWithinPage for gtk, so a signal would be sent to notify location change.

[arno.]

Created attachment 22705 [details]
first attempt patch

[arno.]

Created attachment 22706 [details]
second attempt

emit signal on frame, and on view only if we're in the main frame

[Jan Alonzo]

Hi arno

Is your patch for review? If so can you please change the flag to '?' so it will go to the review queue?

Thanks for the patch!

[arno.]

Comment on attachment 22706 [details]
second attempt

I was waiting for bug 17066, but may be we can still fix this one before

[Holger Freyther]

Comment on attachment 22706 [details]
second attempt

Let me comment on the style first:


>  void FrameLoaderClient::dispatchDidChangeLocationWithinPage()
>  {
> -    notImplemented();
> +    /* Update the URI
> +     */

// Update the URI

or

/*
 * Update the URI
 */


> +    if (m_frame == webkit_web_view_get_main_frame(webView))
> +        g_signal_emit_by_name(webView, "location-changed-within-page", m_frame);
>  }

early exits (like found in many places):

if (m_frame != webkit_web_get_main_frame(webView))
   return;

g_signal_emit_by_name(webView, "...

[arno.]

Created attachment 25028 [details]
style update

Thanks for your comments. Here is an updated patch

[Christian Dywan]

Do we really need a separate signal signal for this? In bug 14807 I'm adding notifications for the URI and title properties, so it should be sufficient to monitor the URI. It is easily distiguishable since no loading or progress information would change.

[Gustavo Noronha (kov)]

Comment on attachment 25028 [details]
style update

I agree with Christian in this matter - there is no need to create a new signal for this, let's just use the properties, and have them notified at dispatchDidChangeLocationWithinPage.