Bug 43157 – First pass at visited link support for WK2

• Component: New Bugs
• Status: RESOLVED FIXED

[Brady Eidson]

Add ability for WK2 to ask client app to populateVisitedLinks

[Brady Eidson]

First pass at visited link support for WK2

-WK2 needs to be able to ask the client app to populate visited links
-The UIProcess needs to be able to tell the WebProcess to track visited links

[Brady Eidson]

Created attachment 62891 [details]
Patch

[WebKit Review Bot]

Attachment 62891 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKContext.h:53:  Extra space between int and version  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:55:  Extra space between WKContextDidNavigateWithNavigationDataCallback and didNavigateWithNavigationData  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:56:  Extra space between WKContextDidPerformClientRedirectCallback and didPerformClientRedirect  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:57:  Extra space between WKContextDidPerformServerRedirectCallback and didPerformServerRedirect  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:58:  Extra space between WKContextDidUpdateHistoryTitleCallback and didUpdateHistoryTitle  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:59:  Extra space between WKContextPopulateVisitedLinksCallback and populateVisitedLinks  [whitespace/declaration] [3]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 62891 [details]
Patch

> +
> +void WebHistoryClient::populateVisitedLinks()
> +{
> +    if (!m_contextHistoryClient.populateVisitedLinks)
> +        return;
> +
> +    m_contextHistoryClient.populateVisitedLinks(toRef(&m_context), m_contextHistoryClient.clientInfo);
>  }

Instead of storing the context here, I think it makes more sense to just pass the context the this method as we do everywhere else.
> -            didNavigateWithNavigationData(webFrame(frameID), store);
> +            pageNamespace()->context()->didNavigateWithNavigationData(webFrame(frameID), store);

I don't think there is any reason for these messages to go to the WebPageProxy, they should be able to go right to the WebContext from the WebProcessProxy.


r- to address these issues.

[Brady Eidson]

Created attachment 62896 [details]
Patch

[WebKit Review Bot]

Attachment 62896 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKContext.h:53:  Extra space between int and version  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:55:  Extra space between WKContextDidNavigateWithNavigationDataCallback and didNavigateWithNavigationData  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:56:  Extra space between WKContextDidPerformClientRedirectCallback and didPerformClientRedirect  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:57:  Extra space between WKContextDidPerformServerRedirectCallback and didPerformServerRedirect  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:58:  Extra space between WKContextDidUpdateHistoryTitleCallback and didUpdateHistoryTitle  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKContext.h:59:  Extra space between WKContextPopulateVisitedLinksCallback and populateVisitedLinks  [whitespace/declaration] [3]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 62896 [details]
Patch

>  void WebContext::ensureWebProcess()
>  {
>      if (m_process && m_process->isValid())
>          return;

Why not use hasValidProcess() here, since you added it?

> +    m_historyClient.didNavigateWithNavigationData(this, frame->page(), store, frame);

Can page() be 0? What should we do in that case?

> +    inline bool hasValidProcess() const { return m_process && m_process->isValid(); }

No need for the "inline" here. Functions defined inside the class definition are automatically treated as if "inline" was specified.

>      WebContextInjectedBundleClient m_injectedBundleClient;
>  
> +    WebHistoryClient m_historyClient;
> +
>      PluginInfoStore m_pluginInfoStore;

Are there some other clients this can be paragraphed with so it doesn't need its own?

> +    if (wkClient && !wkClient->version)
> +        toWK(contextRef)->initializeHistoryClient(wkClient);

I'm a little mystified by the version checks. It seems that if someone passes in version 1 or greater of the structure we'll ignore it completely. That does not seem to be what version is for. This is probably not new to your patch -- maybe we're doing this throughout.

> +                PageGroup::setShouldTrackVisitedLinks(shouldTrackVisitedLinks);

Perhaps setting this to false should clear the existing visited links?

[Brady Eidson]

(In reply to comment #7)
> (From update of attachment 62896 [details])
> >  void WebContext::ensureWebProcess()
> >  {
> >      if (m_process && m_process->isValid())
> >          return;
> 
> Why not use hasValidProcess() here, since you added it?

Indeed!

> 
> > +    m_historyClient.didNavigateWithNavigationData(this, frame->page(), store, frame);
> 
> Can page() be 0? What should we do in that case?

It can't be - We actually translated a frameID to a WebFrame by asking a WebPage before calling this method.

But I'm adding assertions to this effect.

> 
> >      WebContextInjectedBundleClient m_injectedBundleClient;
> >  
> > +    WebHistoryClient m_historyClient;
> > +
> >      PluginInfoStore m_pluginInfoStore;
> 
> Are there some other clients this can be paragraphed with so it doesn't need its own?

The injectedbundleclient is the only other client, but I felt it made slightly more sense to paragraph it with its companion m_injectedBundlePath

> 
> > +    if (wkClient && !wkClient->version)
> > +        toWK(contextRef)->initializeHistoryClient(wkClient);
> 
> I'm a little mystified by the version checks. It seems that if someone passes in version 1 or greater of the structure we'll ignore it completely. That does not seem to be what version is for. This is probably not new to your patch -- maybe we're doing this throughout.

Not new to my patch, we do it throughout - I'm equally mystified.

> 
> > +                PageGroup::setShouldTrackVisitedLinks(shouldTrackVisitedLinks);
> 
> Perhaps setting this to false should clear the existing visited links?

WebCore does this already.

[Brady Eidson]

Committed r64247: <http://trac.webkit.org/changeset/64247>