Android WebView visited link highlighting implementation part 1

android_webview/browser/renderer_host/aw_render_view_host_ext.cc

Index: android_webview/browser/renderer_host/aw_render_view_host_ext.cc
diff --git a/android_webview/browser/renderer_host/aw_render_view_host_ext.cc b/android_webview/browser/renderer_host/aw_render_view_host_ext.cc
index 73bcad0c5482ac625d9af265876a9950c29aa6cd..d3037f5ff89878ee397ea800a8df2ca53e022c98 100644
--- a/android_webview/browser/renderer_host/aw_render_view_host_ext.cc
+++ b/android_webview/browser/renderer_host/aw_render_view_host_ext.cc
@@ -4,6 +4,7 @@
 
 #include "android_webview/browser/renderer_host/aw_render_view_host_ext.h"
 
+#include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/common/render_view_messages.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/callback.h"
@@ -11,6 +12,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/frame_navigate_params.h"
 
 namespace android_webview {
 
@@ -71,6 +73,15 @@ void AwRenderViewHostExt::RenderViewGone(base::TerminationStatus status) {
   }
 }
 
+void AwRenderViewHostExt::DidNavigateAnyFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  DCHECK(CalledOnValidThread());
+
+  AwBrowserContext::FromBrowserContext(
+      web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);

[benm (inactive), 2013/01/16 01:00:22]

interesting, so we will alter the visited history as we navigate, beyond what
the ChromeClient provides. Is this what the old WebView did?

It seems like a sensible thing to do, just wondering what the old behavior was.
and I guess this will do it for iframe navigations? Perhaps we only want main
frame navigations?

[joth, 2013/01/16 01:10:02]

this is a slightly awkward dereferencing chain. It would be nicer if
AwRenderViewHostExt were given the VisitedLinkMaster it should deal with, or
even nicer if the BrowserContext had its own internal WebContentsObserver to
deal with these changes, but I think both those options will involve even bigger
restructuring (probably pulling the AwDependencyFactory too). Thoughts?

[boliu, 2013/01/16 01:10:08]

This is mostly copied from chrome implementation.

I have no idea if webkit will add navigated urls automatically internally or if
android does it, will double check that.

But I think recording iframe navigation makes a lot of sense, I mean from
experience, iframe navigation should not be any different from top level
navigation for link highlighting purpose.

[boliu, 2013/01/16 01:35:34]

I think this is fine

On 2013/01/16 01:10:02, joth wrote:
It feels more natural to have VisitedLinkMaster be owned and accessed through
the browser context, which (kind of) mirrors chrome/ code. Also the second part
of this will have aw_contents poke AwBrowserContext instead of this class.

We could have this class have a pointer to AwBrowserContext directly, but that
doesn't seem to match with convention.
That seems more weird having BrowserContext have explicit knowledge about what
web contents have been created/destroyed (ie where you register the observer)

Incidentally this is exactly what VisitedLinkMaster does and is one of the
things I wanted to factor out of it. It's also why we have to go through the
AwBrowserMainParts::PreMainMessageLoopRun dance to create it.

[boliu, 2013/01/16 01:39:20]

Oh, what we could have is AwBrowserContext::FromWebContents. At least makes this
read easier, I'll do that

[boliu, 2013/01/16 01:58:44]

Use AwBrowserContext::FromWebContents. Lots of time to investigate the old
webkit behavior since this is the 3rd patch in a chain of patches...

+}
+
 bool AwRenderViewHostExt::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AwRenderViewHostExt, message)