Add render frame observer for CopylessPaste

Add render frame observer for CopylessPaste

Extract structured metadata right after the main frame is parsed, and
send to CopylessPaste mojo service if the result is non-empty.

BUG=693650

[wychen, 2017-02-21 23:01:35]

wychen@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org

[wychen, 2017-02-21 23:01:35]

PTAL.

Is there a better place inside WebKit than DidMeaningfulLayout() to put the
entry point?

[wychen, 2017-02-22 00:24:31]

Description was changed from

==========
Add render frame observer for CopylessPaste

Extract structured metadata right after the main frame is parsed, and
send to CopylessPaste mojo service if the result is non-empty.

BUG=693650
==========

to

==========
Add render frame observer for CopylessPaste

Extract structured metadata right after the main frame is parsed, and
send to CopylessPaste mojo service if the result is non-empty.

BUG=693650
==========

[esprehn, 2017-02-22 01:38:55]

Can we do all of this inside blink instead? I'd rather not add more plumbing out
like this.

[wychen, 2017-02-22 01:43:01]

On 2017/02/22 01:38:55, esprehn wrote:
> Can we do all of this inside blink instead? I'd rather not add more plumbing
out
> like this.

Totally agree. Could you point out where I should look or show me an example?

[wychen, 2017-03-30 05:51:40]

On 2017/02/22 01:43:01, wychen wrote:
> On 2017/02/22 01:38:55, esprehn wrote:
> > Can we do all of this inside blink instead? I'd rather not add more plumbing
> out
> > like this.
> 
> Totally agree. Could you point out where I should look or show me an example?

I tried the other way around.
https://codereview.chromium.org/2787703002/ adds the mojo service inside Blink.
The thing is, there's currently no way to initiate a mojo client from Java
without JNI.
Routing this through JNI looks like even more plumbing than the mechanism here.

WDYT if we keep the Mojo service on the Java side?

[dcheng, 2017-04-02 05:50:34]

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
File components/copyless_paste/renderer/copyless_paste_agent.cc (right):

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
components/copyless_paste/renderer/copyless_paste_agent.cc:14: using namespace
blink;
Nit: forbidden by the Google C++ style guide

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
components/copyless_paste/renderer/copyless_paste_agent.cc:24: if
(!render_frame()->IsMainFrame())
Nit: we should just not install a RFO for a non-main frame.

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
components/copyless_paste/renderer/copyless_paste_agent.cc:28: if (doc.isNull()
|| doc.body().isNull())
If we're attached, WebDocument should never be null.

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
File components/copyless_paste/renderer/copyless_paste_agent.h (right):

https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
components/copyless_paste/renderer/copyless_paste_agent.h:16:
CopylessPasteAgent(content::RenderFrame* render_frame);
nit: explicit

https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebDocument.h (right):

https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebDocument.h:141: BLINK_EXPORT void
extractMetadata();
We would prefer not to add new methods to WebDocument, so if possible, I think
we should be trying to make this work with the Mojo service in Blink.

[haraken, 2017-04-03 00:20:18]

On 2017/04/02 05:50:34, dcheng wrote:
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> File components/copyless_paste/renderer/copyless_paste_agent.cc (right):
> 
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> components/copyless_paste/renderer/copyless_paste_agent.cc:14: using namespace
> blink;
> Nit: forbidden by the Google C++ style guide
> 
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> components/copyless_paste/renderer/copyless_paste_agent.cc:24: if
> (!render_frame()->IsMainFrame())
> Nit: we should just not install a RFO for a non-main frame.
> 
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> components/copyless_paste/renderer/copyless_paste_agent.cc:28: if
(doc.isNull()
> || doc.body().isNull())
> If we're attached, WebDocument should never be null.
> 
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> File components/copyless_paste/renderer/copyless_paste_agent.h (right):
> 
>
https://codereview.chromium.org/2709893002/diff/1/components/copyless_paste/r...
> components/copyless_paste/renderer/copyless_paste_agent.h:16:
> CopylessPasteAgent(content::RenderFrame* render_frame);
> nit: explicit
> 
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> File third_party/WebKit/public/web/WebDocument.h (right):
> 
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> third_party/WebKit/public/web/WebDocument.h:141: BLINK_EXPORT void
> extractMetadata();
> We would prefer not to add new methods to WebDocument, so if possible, I think
> we should be trying to make this work with the Mojo service in Blink.

Yeah, maybe should we also consider not adding copyless_paste_agent?
Alternatively we can consider letting the browser process directly request the
renderer to extract metadata via Mojo.

[wychen, 2017-04-03 22:06:38]

On 2017/04/03 00:20:18, haraken wrote:
> On 2017/04/02 05:50:34, dcheng wrote:
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> > File third_party/WebKit/public/web/WebDocument.h (right):
> > 
> >
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> > third_party/WebKit/public/web/WebDocument.h:141: BLINK_EXPORT void
> > extractMetadata();
> > We would prefer not to add new methods to WebDocument, so if possible, I
think
> > we should be trying to make this work with the Mojo service in Blink.
> 
> Yeah, maybe should we also consider not adding copyless_paste_agent?
> Alternatively we can consider letting the browser process directly request the
> renderer to extract metadata via Mojo.

The alternative method was added here:
https://codereview.chromium.org/2787703002/
However, the Mojo client would be in Java, and we have to route it through JNI,
which is in my opinion uglier than adding method to WebDocument.
That's why I'm revisiting the idea of copyless_paste_agent.

[haraken, 2017-04-04 06:29:34]

On 2017/04/03 22:06:38, wychen wrote:
> On 2017/04/03 00:20:18, haraken wrote:
> > On 2017/04/02 05:50:34, dcheng wrote:
> >
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> > > File third_party/WebKit/public/web/WebDocument.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2709893002/diff/1/third_party/WebKit/public/w...
> > > third_party/WebKit/public/web/WebDocument.h:141: BLINK_EXPORT void
> > > extractMetadata();
> > > We would prefer not to add new methods to WebDocument, so if possible, I
> think
> > > we should be trying to make this work with the Mojo service in Blink.
> > 
> > Yeah, maybe should we also consider not adding copyless_paste_agent?
> > Alternatively we can consider letting the browser process directly request
the
> > renderer to extract metadata via Mojo.
> 
> The alternative method was added here:
> https://codereview.chromium.org/2787703002/
> However, the Mojo client would be in Java, and we have to route it through
JNI,
> which is in my opinion uglier than adding method to WebDocument.
> That's why I'm revisiting the idea of copyless_paste_agent.

I think that CL makes more sense. Would you help me understand how ugly would it
be to implement the server in the Java side?

From the renderer perspective, we want to implement as many renderer-side things
in Blink as possible by letting Blink directly talk with the browser process
using Mojo. That CL is following the pattern.

[esprehn, 2017-04-07 00:12:04]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org
- esprehn@chromium.org

[wychen, 2017-04-20 19:07:00]

We ended up putting the Mojo server inside Blink. Closing this CL.