Summary: | First pass at visited link support for WK2 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
Component: | New Bugs | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Brady Eidson
2010-07-28 16:24:42 PDT
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 Created attachment 62891 [details]
Patch
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.
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. Created attachment 62896 [details]
Patch
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.
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? (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. Committed r64247: <http://trac.webkit.org/changeset/64247> |