Android WebView visited link highlighting implementation part 1

Android WebView visited link highlighting implementation

Hooking up visited link component, but still need to hook up
initialization with WebChromeClient.getVisitedHistory. This means
currently, visited links are not persistend or repopulated from
but should work in a single browsing session.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178748

[boliu, 2013-01-11 19:16:38]

Still trying to figure out how to test this. Do you know if js can query if a
link has been visited before?

[mkosiba (inactive), 2013-01-14 11:56:20]

https://codereview.chromium.org/11860014/diff/9/android_webview/browser/rende...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/9/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:28: //
persistencing these urls.
nit: persisting

[joth, 2013-01-14 23:40:35]

lgtm

https://codereview.chromium.org/11860014/diff/7001/android_webview/browser/re...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/7001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:102: //
Persistence of VisitedLink database is handled by the embedding app.
would it be possible/useful to mention which function/call-path provides this
functionality for webview?

https://codereview.chromium.org/11860014/diff/7001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:122: // path is
not used.
(ditto, ish. i.e. how does the restore from disk case flow happen in webview?)

[boliu, 2013-01-16 00:32:00]

PTAL

Moved VisitedLinkMaster to be owned by AwBrowserContext since it should
definitely be per browser_context, not webview.

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

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
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?

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
android_webview/native/aw_contents.h:20: class GURL;
needed?

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

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/a...
android_webview/browser/aw_browser_context.h:73: void AddVisitedURLs(const
std::vector<GURL>& urls);
nit: put this up before all the virtual overrides.

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
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:07]

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
On 2013/01/16 01:00:22, benm wrote:
> 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?

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.

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
android_webview/native/aw_contents.h:20: class GURL;
On 2013/01/16 01:00:22, benm wrote:
> needed?

Guess not, probably already transitively included.

Can't remember why I was modifying this file in the first place...

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

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
I think this is fine

On 2013/01/16 01:10:02, joth wrote:
> this is a slightly awkward dereferencing chain. It would be nicer if
> AwRenderViewHostExt were given the VisitedLinkMaster it should deal with, or

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.

> 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?

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]

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
On 2013/01/16 01:35:34, boliu wrote:
> I think this is fine
> 
> On 2013/01/16 01:10:02, joth wrote:
> > this is a slightly awkward dereferencing chain. It would be nicer if
> > AwRenderViewHostExt were given the VisitedLinkMaster it should deal with, or
> 
> 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.
> 
> > 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?
> 
> 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.

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

[joth, 2013-01-16 01:41:55]

SGTM (and lgtm with that change)


On 15 January 2013 17:39, <boliu@chromium.org> wrote:

>
> https://codereview.chromium.**org/11860014/diff/10001/**
>
android_webview/browser/**renderer_host/aw_render_view_**host_ext.cc<https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/renderer_host/aw_render_view_host_ext.cc>
> File android_webview/browser/**renderer_host/aw_render_view_**host_ext.cc
> (right):
>
> https://codereview.chromium.**org/11860014/diff/10001/**
> android_webview/browser/**renderer_host/aw_render_view_**
>
host_ext.cc#newcode82<https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/renderer_host/aw_render_view_host_ext.cc#newcode82>
> android_webview/browser/**renderer_host/aw_render_view_**host_ext.cc:82:
> web_contents()->**GetBrowserContext())->**AddVisitedURL(params.base_url)**
> ;
> On 2013/01/16 01:35:34, boliu wrote:
>
>> I think this is fine
>>
>
>  On 2013/01/16 01:10:02, joth wrote:
>> > this is a slightly awkward dereferencing chain. It would be nicer if
>> > AwRenderViewHostExt were given the VisitedLinkMaster it should deal
>>
> with, or
>
>  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.
>>
>
>  > 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?
>
>  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.
>>
>
> Oh, what we could have is AwBrowserContext::**FromWebContents. At least
> makes this read easier, I'll do that
>
>
https://codereview.chromium.**org/11860014/<https://codereview.chromium.org/1...
>
> --
>
>
>

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

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/a...
android_webview/browser/aw_browser_context.h:73: void AddVisitedURLs(const
std::vector<GURL>& urls);
On 2013/01/16 01:10:02, joth wrote:
> nit: put this up before all the virtual overrides.

Done, and cc file too

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:82:
web_contents()->GetBrowserContext())->AddVisitedURL(params.base_url);
Use AwBrowserContext::FromWebContents. Lots of time to investigate the old
webkit behavior since this is the 3rd patch in a chain of patches...

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11860014/diff/10001/android_webview/native/aw...
android_webview/native/aw_contents.h:20: class GURL;
On 2013/01/16 01:10:08, boliu wrote:
> On 2013/01/16 01:00:22, benm wrote:
> > needed?
> 
> Guess not, probably already transitively included.
> 
> Can't remember why I was modifying this file in the first place...

Done.

[benm (inactive), 2013-01-16 02:25:45]

lgtm

[commit-bot: I haz the power, 2013-01-25 00:55:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11860014/22001

[commit-bot: I haz the power, 2013-01-25 03:57:34]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-01-25 04:00:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11860014/22001

[commit-bot: I haz the power, 2013-01-25 04:38:42]

Change committed as 178748