Add WebDocument::isMobileFriendly() to blink

Add WebDocument::isMobileFriendly() to blink

BUG=509869

[wychen, 2015-11-01 19:40:39]

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

[wychen, 2015-11-01 19:40:40]

PTAL. This is to provide easier access for the renderer side to know whether a
page is mobile friendly.

[dglazkov, 2015-11-01 20:41:41]

On 2015/11/01 at 19:40:40, wychen wrote:
> PTAL. This is to provide easier access for the renderer side to know whether a
page is mobile friendly.

That's... already on the renderer side? Can you help me better understand the
context of this change? How will this be used?

[wychen, 2015-11-01 23:01:57]

On 2015/11/01 20:41:41, dglazkov wrote:
> That's... already on the renderer side? Can you help me better understand the
> context of this change? How will this be used?

Thanks for your prompt reply!

Sorry, I meant easier for the render process on the Chromium side.

I intent to use this piece of code like this:
https://codereview.chromium.org/1248643004/diff/280001/components/dom_distill...

[dglazkov, 2015-11-02 17:52:19]

On 2015/11/01 at 23:01:57, wychen wrote:
> On 2015/11/01 20:41:41, dglazkov wrote:
> > That's... already on the renderer side? Can you help me better understand
the
> > context of this change? How will this be used?
> 
> Thanks for your prompt reply!
> 
> Sorry, I meant easier for the render process on the Chromium side.
> 
> I intent to use this piece of code like this:
>
https://codereview.chromium.org/1248643004/diff/280001/components/dom_distill...

I see. Putting this function on WebDocument is not right. This is more of a
viewport concept and it doesn't make sense on a Document (or WebDocument). There
could be (and usually are) multiple Document instances in a viewport.

Maybe it would be better to have this logic all on the other side of the
Blink/WebKit API, and the content/renderer logic will just be plumbing things
out?

[wychen, 2015-11-02 18:05:18]

On 2015/11/02 17:52:19, dglazkov wrote:
> I see. Putting this function on WebDocument is not right. This is more of a
> viewport concept and it doesn't make sense on a Document (or WebDocument).
There
> could be (and usually are) multiple Document instances in a viewport.
> 
> Maybe it would be better to have this logic all on the other side of the
> Blink/WebKit API, and the content/renderer logic will just be plumbing things
> out?

How about adding one bool field isMobileFriendly to WebDistillabilityFeatures,
and
if it is mobile friendly, only set this field and return early.

Then in distillability_agent.cc, only do the classification when it is not
mobile friendly.

Is this what you meant?

[dglazkov, 2015-11-02 18:15:06]

On 2015/11/02 at 18:05:18, wychen wrote:
> On 2015/11/02 17:52:19, dglazkov wrote:
> > I see. Putting this function on WebDocument is not right. This is more of a
> > viewport concept and it doesn't make sense on a Document (or WebDocument).
There
> > could be (and usually are) multiple Document instances in a viewport.
> > 
> > Maybe it would be better to have this logic all on the other side of the
> > Blink/WebKit API, and the content/renderer logic will just be plumbing
things
> > out?
> 
> How about adding one bool field isMobileFriendly to WebDistillabilityFeatures,
and
> if it is mobile friendly, only set this field and return early.
> 
> Then in distillability_agent.cc, only do the classification when it is not
mobile friendly.
> 
> Is this what you meant?

That seems better, let's start with that.

[wychen, 2015-11-02 18:55:32]

On 2015/11/02 18:15:06, dglazkov wrote:
> That seems better, let's start with that.

The Blink side is done here:
https://codereview.chromium.org/1419033004/#ps210001

Could you take a look over there? Thanks!

I'll update you when the calling side is done.

[wychen, 2015-11-04 19:17:39]

On 2015/11/02 18:55:32, wychen wrote:
> On 2015/11/02 18:15:06, dglazkov wrote:
> > That seems better, let's start with that.
> 
> The Blink side is done here:
> https://codereview.chromium.org/1419033004/#ps210001
> 
> Could you take a look over there? Thanks!
> 
> I'll update you when the calling side is done.

The calling side is here:
https://codereview.chromium.org/1434433002/

[esprehn, 2015-11-05 01:10:50]

On 2015/11/04 at 19:17:39, wychen wrote:
> On 2015/11/02 18:55:32, wychen wrote:
> > On 2015/11/02 18:15:06, dglazkov wrote:
> > > That seems better, let's start with that.
> > 
> > The Blink side is done here:
> > https://codereview.chromium.org/1419033004/#ps210001
> > 
> > Could you take a look over there? Thanks!
> > 
> > I'll update you when the calling side is done.
> 
> The calling side is here:
> https://codereview.chromium.org/1434433002/

This is not used anymore in that patch, I thought we put it in the Features
object? I don't think we want this on WebDocument.

[wychen, 2015-11-05 01:17:31]

On 2015/11/05 01:10:50, esprehn wrote:
> On 2015/11/04 at 19:17:39, wychen wrote:
> > On 2015/11/02 18:55:32, wychen wrote:
> > > On 2015/11/02 18:15:06, dglazkov wrote:
> > > > That seems better, let's start with that.
> > > 
> > > The Blink side is done here:
> > > https://codereview.chromium.org/1419033004/#ps210001
> > > 
> > > Could you take a look over there? Thanks!
> > > 
> > > I'll update you when the calling side is done.
> > 
> > The calling side is here:
> > https://codereview.chromium.org/1434433002/
> 
> This is not used anymore in that patch, I thought we put it in the Features
> object? I don't think we want this on WebDocument.

Sorry for the confusion. This CL was discarded, and the functionality was
rewritten in https://codereview.chromium.org/1419033004/#ps210001 (patch set 8).
The calling side of the rewritten code is
https://codereview.chromium.org/1434433002/.

I'll close this CL now.