Bug 20306 - [gtk] implements FrameLoaderClient::dispatchDidChangeLocationWithinPage
Summary: [gtk] implements FrameLoaderClient::dispatchDidChangeLocationWithinPage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 17066
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-06 13:35 PDT by arno.
Modified: 2009-05-06 16:36 PDT (History)
3 users (show)

See Also:


Attachments
first attempt patch (2.85 KB, patch)
2008-08-08 05:47 PDT, arno.
no flags Details | Formatted Diff | Diff
second attempt (4.41 KB, patch)
2008-08-08 08:11 PDT, arno.
no flags Details | Formatted Diff | Diff
style update (4.43 KB, patch)
2008-11-10 14:47 PST, arno.
gustavo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2008-08-06 13:35:10 PDT
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.
Comment 1 arno. 2008-08-08 05:47:58 PDT
Created attachment 22705 [details]
first attempt patch
Comment 2 arno. 2008-08-08 08:11:07 PDT
Created attachment 22706 [details]
second attempt

emit signal on frame, and on view only if we're in the main frame
Comment 3 Jan Alonzo 2008-09-23 05:27:10 PDT
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!
Comment 4 arno. 2008-09-26 04:23:17 PDT
Comment on attachment 22706 [details]
second attempt

I was waiting for bug 17066, but may be we can still fix this one before
Comment 5 Holger Freyther 2008-11-09 15:26:25 PST
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, "...
Comment 6 arno. 2008-11-10 14:47:28 PST
Created attachment 25028 [details]
style update

Thanks for your comments. Here is an updated patch
Comment 7 Christian Dywan 2009-01-24 21:54:15 PST
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.
Comment 8 Gustavo Noronha (kov) 2009-04-20 08:02:07 PDT
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.