Implement WebPrescientNetworking to trigger preconnect from Blink

Implement WebPrescientNetworking to trigger preconnect from blink.

Note to gardener: This patch requires the following Blink patch to be landed first:
  https://codereview.chromium.org/14746002/

This API is to be triggered from {mouse,gesture}-events on HTMLAnchorElement. 

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

[kouhei (in TOK), 2013-05-01 06:04:24]

Could you review this patch?

[kouhei (in TOK), 2013-05-01 06:13:02]

Few notes:

The paramaeter "motivationVariationId" is to experiment minor trigger
variations.
For {mouse,touch}-event prefetch, I want to use it to pass link width/area
information to UMA histograms.

The "pretend-to-preconnect" patch is not included. This means that preconnect w/
mouse,touch event motivations will actually trigger preconnect. However,
currently there is no code invoking the trigger in Blink's HTMLAnchorElement, so
it should not be a problem. I plan to  post a WIP patch for
"pretend-to-preconnect" within a few days.

[mmenke, 2013-05-01 14:39:46]

[+jar]  I am not familiar with this code.  I believe jar is.

Also, it's generally better to have a domain owner review a CL first, and wait
for his sign off, before sending it to a wider audience.

Then add the other owners as reviewers, and post a message telling each
specifically what you want them to review.

[jam, 2013-05-01 14:58:10]

https://codereview.chromium.org/14749005/diff/1/content/public/renderer/conte...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/1/content/public/renderer/conte...
content/public/renderer/content_renderer_client.h:211: uint32_t
motivationVariationId) {}
nit: google c++ naming style (i.e. motivation_variation_id)

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
content/renderer/renderer_webkitplatformsupport_impl.h:56: virtual void
preconnect(const WebKit::WebURL& url,
this method and the above (prefetchHostName) are just pass-throughs to the
embedder. it seems that coneptually they're related to, although I realize
aren't the same thing as, prerendering. can we put them on WebPrerendererClient?
that way we wouldn't have to change content for these chrome-specific features.
we can rename the interface to something more generic if needed

[kouhei (in TOK), 2013-05-02 09:50:43]

> Also, it's generally better to have a domain owner review a CL first, and wait
> for his sign off, before sending it to a wider audience.
> 
> Then add the other owners as reviewers, and post a message telling each
> specifically what you want them to review.

Thank you for your advice. I will follow that next time.

[kouhei (in TOK), 2013-05-02 10:15:25]

Thank you for your comments.

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
content/renderer/renderer_webkitplatformsupport_impl.h:56: virtual void
preconnect(const WebKit::WebURL& url,
On 2013/05/01 14:58:11, jam wrote:
> this method and the above (prefetchHostName) are just pass-throughs to the
> embedder. it seems that coneptually they're related to, although I realize
> aren't the same thing as, prerendering. can we put them on
WebPrerendererClient?
> that way we wouldn't have to change content for these chrome-specific
features.
> we can rename the interface to something more generic if needed

prefetchHostName was not a chrome specific feature (in WebKit days), but I think
preconnect is a chrome specific feature.

I agree that all these conceptually related prefetch methods (prefetchHostName,
preconnect, prerender) should be organlized.

I'll try if these would fit in WebPrerendererClient.

[jar (doing other things), 2013-05-04 00:32:05]

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor.cc
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor....
chrome/browser/net/predictor.cc:281:
BrowserThread::CurrentlyOn(BrowserThread::IO));
I need to check... but the old assertion suggests this MUST be on the UI thread.
 I think that the class members can only be accessed by that thread.  I need to
understand how you have changed that so as to be safe.

After reading the rest of the CL, I *think* you should not be calling this
directly, and should instead be doing a post task to get on the UI thread (when
handling the IPC message from the renderer).

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor....
chrome/browser/net/predictor.cc:282: if (!predictor_enabled_)
This is vaguely thread safe (it is "only" a bool :-/ ), but should trigger
thread sanitizer issues if we ever had a test that toggles the network
prediction for this profile (I think).  I would expect this to be toggled on the
UI thread, but now <sadly> tested on the IO thread (sometimes?).

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor....
chrome/browser/net/predictor.cc:293: url_request_context_getter_);
I have to read the code more to see that your access to this member is also
thread safe.

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/url_info.h
File chrome/browser/net/url_info.h (right):

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/url_info.h...
chrome/browser/net/url_info.h:56: TAP_DOWN_MOTIVATED, // Tap-down link induced
preconnect.
nit: indent comments to align them (style as in lines 38-41).

[kouhei (in TOK), 2013-05-07 09:16:12]

Updated patch.
jam, jar: Could you take a look?

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor.cc
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/predictor....
chrome/browser/net/predictor.cc:282: if (!predictor_enabled_)
On 2013/05/04 00:32:05, jar wrote:
> This is vaguely thread safe (it is "only" a bool :-/ ), but should trigger
> thread sanitizer issues if we ever had a test that toggles the network
> prediction for this profile (I think).  I would expect this to be toggled on
the
> UI thread, but now <sadly> tested on the IO thread (sometimes?).

Actually, some methods on this class is already called from both UI and IO
thread. For this specific method, I prefer to have this run on IO thread, as on
the UI thread, the PreconnectOnUIThread will just bounce to IO thread.
cf.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

The variable predictor_enabled_ and url_request_context_getter_ is used on both
UI and IO thread from what I see on other methods.
There seems no problem in using url_request_context_getter_, as it is
initialized once in InitNetworkPredictor, and never touched again.
predictor_enabled_ is actually explicitly modified on IO thread, so referencing
on the IO thread is fine.
However, the original version of this method (and many other methods in this
class) does READ access to this boolean flag from UI thread.

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/url_info.h
File chrome/browser/net/url_info.h (right):

https://codereview.chromium.org/14749005/diff/1/chrome/browser/net/url_info.h...
chrome/browser/net/url_info.h:56: TAP_DOWN_MOTIVATED, // Tap-down link induced
preconnect.
On 2013/05/04 00:32:05, jar wrote:
> nit: indent comments to align them (style as in lines 38-41).

Done.

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web...
content/renderer/renderer_webkitplatformsupport_impl.h:56: virtual void
preconnect(const WebKit::WebURL& url,
Updated the patch to use WebPrerenderingSupport instead of the platform API.

[jam, 2013-05-07 16:22:35]

https://codereview.chromium.org/14749005/diff/12001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/12001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:219: uint32_t
motivationVariationId) {}
are changes to this file leftover?

[kouhei (in TOK), 2013-05-07 16:30:46]

https://codereview.chromium.org/14749005/diff/12001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/12001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:219: uint32_t
motivationVariationId) {}
On 2013/05/07 16:22:36, jam wrote:
> are changes to this file leftover?
Yes. This needs to be removed.
Thank you for pointing it out.

[jar (doing other things), 2013-05-08 01:34:25]

predictor.* LGTM with nit shown below.

https://codereview.chromium.org/14749005/diff/12001/chrome/browser/net/predic...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/14749005/diff/12001/chrome/browser/net/predic...
chrome/browser/net/predictor.cc:285: if (preconnect_enabled()) {
nit: please do an early return here, when !preconnect_enabled() to make it
easier to read.

You could make a bigger list of || if you'd like now that we have 3 returns in a
row. (probably should have done this earlier... but your change helps make this
last if-clause large enough that I was having a hard time seeing matching
curlies).

[kouhei (in TOK), 2013-05-08 02:09:38]

gavinp: Could you review the prerender_dispatcher.* ?

Thanks!
removed content/public/renderer/content_renderer_client.h from CL.

https://codereview.chromium.org/14749005/diff/12001/chrome/browser/net/predic...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/14749005/diff/12001/chrome/browser/net/predic...
chrome/browser/net/predictor.cc:285: if (preconnect_enabled()) {
Done. I love early returns.

[gavinp, 2013-05-08 11:16:55]

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.h:44: // From
WebPrerenderingSupport:
Can you add me to the review for the changes to WebPrerenderingSupport in blink?

[kouhei (in TOK), 2013-05-08 13:33:16]

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.h:44: // From
WebPrerenderingSupport:
Blink side is here: https://codereview.chromium.org/14746002/
cbentzel has already added you as reviewer.

[gavinp, 2013-05-09 14:49:16]

lgtm

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:115: ));
These should move up to the previous line.

[kouhei (in TOK), 2013-05-10 01:59:09]

brettw: Could you review chrome/browser/renderer_host* and
chrome/common/render_messages.h?

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/14749005/diff/24001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:115: ));
On 2013/05/09 14:49:16, gavinp wrote:
> These should move up to the previous line.

Done.

[kouhei (in TOK), 2013-05-14 07:46:08]

jochen: Would you review changes in chrome/**?
jam: Would you review changes in content/**?

[kouhei (in TOK), 2013-05-16 01:47:23]

thakis: Would you review changes in chrome/**?
jam: Would you review changes in content/**?

> jochen: Would you review changes in chrome/**?
Sorry. I didn't notice that you are out of office this week.

[Nico, 2013-05-16 04:35:44]

chrome/ lgtm modulo function name

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:109: virtual
WebKit::WebPrescientNetworking* PrescientNetworking() OVERRIDE;
style guide says that functions should be named like verbs

[kouhei (in TOK), 2013-05-16 07:02:50]

brettw: Would you review changes in content/**?

> jam: Would you review changes in content/**?
Sorry. I didn't notice that you are OoO too.

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:109: virtual
WebKit::WebPrescientNetworking* PrescientNetworking() OVERRIDE;
I will change his to getPrescientNetworking().

[Nico, 2013-05-16 18:26:16]

Sorry that I spend so much time on such a trivial matter as a function name :-/

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:109: virtual
WebKit::WebPrescientNetworking* PrescientNetworking() OVERRIDE;
On 2013/05/16 07:02:50, Kouhei_UENO wrote:
> I will change his to getPrescientNetworking().

chromium style is LikeThis (i.e. a capital 'G') (or like_this for simple
getters). getPrescientNetworking is webkit style and doesn't belong here since
it's not overriding something in blink from what i understand.

[kouhei (in TOK), 2013-05-17 01:48:05]

thakis: Thanks for review!

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/14749005/diff/28002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:109: virtual
WebKit::WebPrescientNetworking* PrescientNetworking() OVERRIDE;
On 2013/05/16 18:26:17, Nico wrote:
> On 2013/05/16 07:02:50, Kouhei_UENO wrote:
> > I will change his to getPrescientNetworking().
> 
> chromium style is LikeThis (i.e. a capital 'G') (or like_this for simple
> getters). getPrescientNetworking is webkit style and doesn't belong here since
> it's not overriding something in blink from what i understand.

Done.

[kouhei (in TOK), 2013-05-21 00:52:36]

brettw: Would you review changes in content/**?

Thanks.

[brettw, 2013-05-21 18:03:56]

content lgtm with style nit

https://codereview.chromium.org/14749005/diff/72005/content/public/renderer/c...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/14749005/diff/72005/content/public/renderer/c...
content/public/renderer/content_renderer_client.cc:130:
WebKit::WebPrescientNetworking* ContentRendererClient::GetPrescientNetworking()
{
Line length.

[kouhei (in TOK), 2013-05-21 23:42:39]

Thanks for review!

https://codereview.chromium.org/14749005/diff/72005/content/public/renderer/c...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/14749005/diff/72005/content/public/renderer/c...
content/public/renderer/content_renderer_client.cc:130:
WebKit::WebPrescientNetworking* ContentRendererClient::GetPrescientNetworking()
{
On 2013/05/21 18:03:56, brettw wrote:
> Line length.

Done.

[commit-bot: I haz the power, 2013-05-21 23:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/14749005/84001

[commit-bot: I haz the power, 2013-05-22 00:07:16]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[kouhei (in TOK), 2013-05-22 00:51:15]

cevans: Would you take a look at render_messages.h? I added an IPC to tell
"Predictor" to start pre-connecting.

[kouhei (in TOK), 2013-05-23 00:33:34]

cevans: ping. I appreciate if you could review this patch, as this is blocking
me from submitting several CLs that depend on this.

[Chris Evans, 2013-05-24 18:55:24]

On 2013/05/23 00:33:34, kouhei wrote:
> cevans: ping. I appreciate if you could review this patch, as this is blocking
> me from submitting several CLs that depend on this.

LGTM

[commit-bot: I haz the power, 2013-05-27 00:05:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/14749005/84001

[commit-bot: I haz the power, 2013-05-27 00:05:34]

Failed to apply patch for chrome/renderer/chrome_content_renderer_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/chrome_content_renderer_client.cc
  Hunk #1 succeeded at 41 (offset 1 line).
  Hunk #2 FAILED at 215.
  Hunk #3 succeeded at 1012 (offset -7 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/renderer/chrome_content_renderer_client.cc.rej

Patch:       chrome/renderer/chrome_content_renderer_client.cc
Index: chrome/renderer/chrome_content_renderer_client.cc
diff --git a/chrome/renderer/chrome_content_renderer_client.cc
b/chrome/renderer/chrome_content_renderer_client.cc
index
20a61de7a30b54ac8c3fbd51b69305fa845a4b79..17562122720e63b1b4041e11efa1d7965a44d2e2
100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -40,6 +40,7 @@
 #include "chrome/renderer/external_extension.h"
 #include "chrome/renderer/loadtimes_extension_bindings.h"
 #include "chrome/renderer/net/net_error_helper.h"
+#include "chrome/renderer/net/prescient_networking_dispatcher.h"
 #include "chrome/renderer/net/renderer_net_predictor.h"
 #include "chrome/renderer/net_benchmarking_extension.h"
 #include "chrome/renderer/one_click_signin_agent.h"
@@ -214,6 +215,7 @@ ChromeContentRendererClient::~ChromeContentRendererClient()
{
 void ChromeContentRendererClient::RenderThreadStarted() {
   chrome_observer_.reset(new ChromeRenderProcessObserver(this));
   extension_dispatcher_.reset(new extensions::Dispatcher());
+  prescient_networking_dispatcher_.reset(new PrescientNetworkingDispatcher());
   net_predictor_.reset(new RendererNetPredictor());
   spellcheck_.reset(new SpellCheck());
   visited_link_slave_.reset(new visitedlink::VisitedLinkSlave());
@@ -1018,6 +1020,11 @@ void ChromeContentRendererClient::PrefetchHostName(const
char* hostname,
   net_predictor_->Resolve(hostname, length);
 }
 
+WebKit::WebPrescientNetworking*
+ChromeContentRendererClient::GetPrescientNetworking() {
+  return prescient_networking_dispatcher_.get();
+}
+
 bool ChromeContentRendererClient::ShouldOverridePageVisibilityState(
     const content::RenderView* render_view,
     WebKit::WebPageVisibilityState* override_state) const {

[commit-bot: I haz the power, 2013-05-27 00:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/14749005/98001

[commit-bot: I haz the power, 2013-05-27 05:48:00]

Change committed as 202370

[kouhei (in TOK), 2013-05-27 09:04:04]

Thanks for review!

[jam, 2013-06-03 16:41:28]

https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
content/public/renderer/content_renderer_client.h:217: virtual
WebKit::WebPrescientNetworking* GetPrescientNetworking();
for WebKit classes that are implemented in src/chrome, we usually don't change
content APIs for them. instead we make chrome set the pointers for these classes
directly, i.e. see 
WebKit::WebAutofillClient
WebKit::WebPrerendererClient
WebKit::WebSpellCheckClient
as some examples, and ChromeContentRendererClient::RenderViewCreated which adds
the chrome implementations at runtime

[kouhei (in TOK), 2013-06-04 16:48:29]

On 2013/06/03 16:41:28, jam wrote:
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> File content/public/renderer/content_renderer_client.h (right):
> 
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> content/public/renderer/content_renderer_client.h:217: virtual
> WebKit::WebPrescientNetworking* GetPrescientNetworking();
> for WebKit classes that are implemented in src/chrome, we usually don't change
> content APIs for them. instead we make chrome set the pointers for these
classes
> directly, i.e. see 
> WebKit::WebAutofillClient
> WebKit::WebPrerendererClient
> WebKit::WebSpellCheckClient
> as some examples, and ChromeContentRendererClient::RenderViewCreated which
adds
> the chrome implementations at runtime

I agree we should minimize change to content.
However, I chose to extend Platform as those methods require an accessible
WebView instance.
I wanted to access prefetchDNS/preconnect from core/dom implementation.

WebPrerendererClient is provided via WebView, but the code is complex to invoke
it from core/loader.
My understanding is that this is done because it requires interaction with
WebView (get Frame orientations, create hidden tab, etc.) anyway.

[jam, 2013-06-04 17:08:26]

On 2013/06/04 16:48:29, kouhei wrote:
> On 2013/06/03 16:41:28, jam wrote:
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > File content/public/renderer/content_renderer_client.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > content/public/renderer/content_renderer_client.h:217: virtual
> > WebKit::WebPrescientNetworking* GetPrescientNetworking();
> > for WebKit classes that are implemented in src/chrome, we usually don't
change
> > content APIs for them. instead we make chrome set the pointers for these
> classes
> > directly, i.e. see 
> > WebKit::WebAutofillClient
> > WebKit::WebPrerendererClient
> > WebKit::WebSpellCheckClient
> > as some examples, and ChromeContentRendererClient::RenderViewCreated which
> adds
> > the chrome implementations at runtime
> 
> I agree we should minimize change to content.
> However, I chose to extend Platform as those methods require an accessible
> WebView instance.
> I wanted to access prefetchDNS/preconnect from core/dom implementation.
> 
> WebPrerendererClient is provided via WebView, but the code is complex to
invoke
> it from core/loader.
> My understanding is that this is done because it requires interaction with
> WebView (get Frame orientations, create hidden tab, etc.) anyway.

I'm not sure I follow what you mean. Look at AutofillAgent in
src/components/autofill/renderer/autofill_agent.h It implements the
WebKit::WebAutofillClient interface and is instantiated by
ChromeContentRendererClient::RenderViewCreated. On construction, it registers
itself with the WebView (see GetWebView()->setAutofillClient(this); ). The
pointer is then available through a getter on WebViewImpl, and any code has
access to that.

Why can't WebPrescientNetworking be treated similar to WebAutofillClient?

[kouhei (in TOK), 2013-06-04 21:59:14]

On 2013/06/04 17:08:26, jam wrote:
> On 2013/06/04 16:48:29, kouhei wrote:
> > On 2013/06/03 16:41:28, jam wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > File content/public/renderer/content_renderer_client.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > content/public/renderer/content_renderer_client.h:217: virtual
> > > WebKit::WebPrescientNetworking* GetPrescientNetworking();
> > > for WebKit classes that are implemented in src/chrome, we usually don't
> change
> > > content APIs for them. instead we make chrome set the pointers for these
> > classes
> > > directly, i.e. see 
> > > WebKit::WebAutofillClient
> > > WebKit::WebPrerendererClient
> > > WebKit::WebSpellCheckClient
> > > as some examples, and ChromeContentRendererClient::RenderViewCreated which
> > adds
> > > the chrome implementations at runtime
> > 
> > I agree we should minimize change to content.
> > However, I chose to extend Platform as those methods require an accessible
> > WebView instance.
> > I wanted to access prefetchDNS/preconnect from core/dom implementation.
> > 
> > WebPrerendererClient is provided via WebView, but the code is complex to
> invoke
> > it from core/loader.
> > My understanding is that this is done because it requires interaction with
> > WebView (get Frame orientations, create hidden tab, etc.) anyway.
> 
> I'm not sure I follow what you mean. Look at AutofillAgent in
> src/components/autofill/renderer/autofill_agent.h It implements the
> WebKit::WebAutofillClient interface and is instantiated by
> ChromeContentRendererClient::RenderViewCreated. On construction, it registers
> itself with the WebView (see GetWebView()->setAutofillClient(this); ). The
> pointer is then available through a getter on WebViewImpl, and any code has
> access to that.
> 
> Why can't WebPrescientNetworking be treated similar to WebAutofillClient?

WebAutofillClient is only accessed from Chromium port codes.

Cam WebViewImpl easily accessed from WebCore? I think that's where I have
mistaken.

Only code I know doing this is WebPrerendererClient, which is invoked via
chromium/src/PrerendererClicentImpl,
which implements core/loader/PrerendererClient obtained by Supplement<Page>.
The Supplement<Page> results in hashmap lookup, so it has to be cached
somewhere.

Replicating above for WebPrescientNetworking is possbile, but I thought it was
simpler
to just add a method to Platform.

[jam, 2013-06-10 17:42:12]

On 2013/06/04 21:59:14, kouhei wrote:
> On 2013/06/04 17:08:26, jam wrote:
> > On 2013/06/04 16:48:29, kouhei wrote:
> > > On 2013/06/03 16:41:28, jam wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > > File content/public/renderer/content_renderer_client.h (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > > content/public/renderer/content_renderer_client.h:217: virtual
> > > > WebKit::WebPrescientNetworking* GetPrescientNetworking();
> > > > for WebKit classes that are implemented in src/chrome, we usually don't
> > change
> > > > content APIs for them. instead we make chrome set the pointers for these
> > > classes
> > > > directly, i.e. see 
> > > > WebKit::WebAutofillClient
> > > > WebKit::WebPrerendererClient
> > > > WebKit::WebSpellCheckClient
> > > > as some examples, and ChromeContentRendererClient::RenderViewCreated
which
> > > adds
> > > > the chrome implementations at runtime
> > > 
> > > I agree we should minimize change to content.
> > > However, I chose to extend Platform as those methods require an accessible
> > > WebView instance.
> > > I wanted to access prefetchDNS/preconnect from core/dom implementation.
> > > 
> > > WebPrerendererClient is provided via WebView, but the code is complex to
> > invoke
> > > it from core/loader.
> > > My understanding is that this is done because it requires interaction with
> > > WebView (get Frame orientations, create hidden tab, etc.) anyway.
> > 
> > I'm not sure I follow what you mean. Look at AutofillAgent in
> > src/components/autofill/renderer/autofill_agent.h It implements the
> > WebKit::WebAutofillClient interface and is instantiated by
> > ChromeContentRendererClient::RenderViewCreated. On construction, it
registers
> > itself with the WebView (see GetWebView()->setAutofillClient(this); ). The
> > pointer is then available through a getter on WebViewImpl, and any code has
> > access to that.
> > 
> > Why can't WebPrescientNetworking be treated similar to WebAutofillClient?
> 
> WebAutofillClient is only accessed from Chromium port codes.
> 
> Cam WebViewImpl easily accessed from WebCore? I think that's where I have
> mistaken.
> 
> Only code I know doing this is WebPrerendererClient, which is invoked via
> chromium/src/PrerendererClicentImpl,
> which implements core/loader/PrerendererClient obtained by Supplement<Page>.
> The Supplement<Page> results in hashmap lookup, so it has to be cached
> somewhere.
> 
> Replicating above for WebPrescientNetworking is possbile, but I thought it was
> simpler
> to just add a method to Platform.

why is this interface in WebKit/public/platform/WebPrescientNetworking.h instead
of WebKit/Source/WebKit/chromium/public, since it's implemented by the embedder?
then it would be available for use by core, right?
+darin for his thoughts

[kouhei (in TOK), 2013-06-10 18:07:48]

On 2013/06/10 17:42:12, jam wrote:
> On 2013/06/04 21:59:14, kouhei wrote:
> > On 2013/06/04 17:08:26, jam wrote:
> > > On 2013/06/04 16:48:29, kouhei wrote:
> > > > On 2013/06/03 16:41:28, jam wrote:
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > > > File content/public/renderer/content_renderer_client.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/14749005/diff/98001/content/public/ren...
> > > > > content/public/renderer/content_renderer_client.h:217: virtual
> > > > > WebKit::WebPrescientNetworking* GetPrescientNetworking();
> > > > > for WebKit classes that are implemented in src/chrome, we usually
don't
> > > change
> > > > > content APIs for them. instead we make chrome set the pointers for
these
> > > > classes
> > > > > directly, i.e. see 
> > > > > WebKit::WebAutofillClient
> > > > > WebKit::WebPrerendererClient
> > > > > WebKit::WebSpellCheckClient
> > > > > as some examples, and ChromeContentRendererClient::RenderViewCreated
> which
> > > > adds
> > > > > the chrome implementations at runtime
> > > > 
> > > > I agree we should minimize change to content.
> > > > However, I chose to extend Platform as those methods require an
accessible
> > > > WebView instance.
> > > > I wanted to access prefetchDNS/preconnect from core/dom implementation.
> > > > 
> > > > WebPrerendererClient is provided via WebView, but the code is complex to
> > > invoke
> > > > it from core/loader.
> > > > My understanding is that this is done because it requires interaction
with
> > > > WebView (get Frame orientations, create hidden tab, etc.) anyway.
> > > 
> > > I'm not sure I follow what you mean. Look at AutofillAgent in
> > > src/components/autofill/renderer/autofill_agent.h It implements the
> > > WebKit::WebAutofillClient interface and is instantiated by
> > > ChromeContentRendererClient::RenderViewCreated. On construction, it
> registers
> > > itself with the WebView (see GetWebView()->setAutofillClient(this); ). The
> > > pointer is then available through a getter on WebViewImpl, and any code
has
> > > access to that.
> > > 
> > > Why can't WebPrescientNetworking be treated similar to WebAutofillClient?
> > 
> > WebAutofillClient is only accessed from Chromium port codes.
> > 
> > Cam WebViewImpl easily accessed from WebCore? I think that's where I have
> > mistaken.
> > 
> > Only code I know doing this is WebPrerendererClient, which is invoked via
> > chromium/src/PrerendererClicentImpl,
> > which implements core/loader/PrerendererClient obtained by Supplement<Page>.
> > The Supplement<Page> results in hashmap lookup, so it has to be cached
> > somewhere.
> > 
> > Replicating above for WebPrescientNetworking is possbile, but I thought it
was
> > simpler
> > to just add a method to Platform.
> 
> why is this interface in WebKit/public/platform/WebPrescientNetworking.h
instead
> of WebKit/Source/WebKit/chromium/public, since it's implemented by the
embedder?
> then it would be available for use by core, right?
> +darin for his thoughts

+abarth

I followed abarth's directory re-organization plan. I think both directories are
currently accessible from core.
cf.
https://docs.google.com/a/google.com/document/d/1BCRkA5ERZtBMXkXB1aMFKKLl5LR_...

[abarth-chromium, 2013-06-11 18:31:46]

Long thread is long.  Let me know if I missed the part you wanted me to comment
on.

>>> Can WebViewImpl easily accessed from WebCore?

Nope.

>> why is this interface in WebKit/public/platform/WebPrescientNetworking.h
instead
>> of WebKit/Source/WebKit/chromium/public, since it's implemented by the
embedder?

Most of the interfaces in public/platform are implemented by the embedder.  The
purpose of the Platform API (the headers in public/platform) is to provide
interfaces by which Blink can call into Chromium.

> > then it would be available for use by core, right?

Core is allowed to depend on public/platform, yes.

> I followed abarth's directory re-organization plan. I think both directories
are
> currently accessible from core.

Core cannot access WebKit/Source/WebKit/chromium/public (soon to be
public/webpage).  Core can access public/platform.

[jam, 2013-06-11 20:34:59]

i see, ok thanks for the explanation.