[webkit-reviews] review granted: [Bug 43157] First pass at visited link support for WK2 : [Attachment 62896] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 43157: First pass at visited link support for WK2
https://bugs.webkit.org/show_bug.cgi?id=43157

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

------- Additional Comments from Darin Adler <darin at apple.com>
>  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?