Propagate the correct device scale factor into the RemoteFrame.

Propagate the correct device scale factor into the RemoteFrame.

BUG=522237, 493262
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201936

[lfg, 2015-09-04 19:21:47]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2015-09-04 19:21:47]

Hi Daniel, please take a look!

[lfg, 2015-09-04 19:38:57]

Patchset #1 (id:1) has been deleted

[dcheng, 2015-09-04 20:16:33]

dcheng@chromium.org changed reviewers:
+ bokan@chromium.org

[dcheng, 2015-09-04 20:16:34]

+bokan, who's probably a better reviewer for this

[dcheng, 2015-09-04 20:19:10]

(Also, update the CL description to point to the original bug as well?)

[bokan, 2015-09-07 20:50:15]

I spoke with lfg@ offline briefly about this. I was unsure that
device_scale_factor was the correct thing to pass here. However, taking a second
look I think this is correct. The main consumer of this seems to be expecting
the DSF ( CrossProcessFrameConnector::OnInitializeChildFrame) though there is
another user in HTMLFrame::initializeChildFrame that seems to assume this will
always be 1.
(https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...)
I would leave a TODO there and maybe notify a relevant owner for that code,
chances are it's a no-op but I have no expertise there.

While we're here, could you please also update the WebFrameClient interface and
its descendants to disambiguate the scale parameter in initializeChildFrame().
scale_factor is quite ambiguous and IMO that's led to confusion here, please
name it device_scale_factor so future generations will know what we're talking
about. Also, in your issue description 's/page scale factor/device scale
factor'.

[lfg, 2015-09-08 20:01:23]

On 2015/09/07 20:50:15, bokan wrote:
> I spoke with lfg@ offline briefly about this. I was unsure that
> device_scale_factor was the correct thing to pass here. However, taking a
second
> look I think this is correct. The main consumer of this seems to be expecting
> the DSF ( CrossProcessFrameConnector::OnInitializeChildFrame) though there is
> another user in HTMLFrame::initializeChildFrame that seems to assume this will
> always be 1.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...)
> I would leave a TODO there and maybe notify a relevant owner for that code,
> chances are it's a no-op but I have no expertise there.

The scale factor is ignored by the Mandoline as of today, but that should be
fixed at one point.

> While we're here, could you please also update the WebFrameClient interface
and
> its descendants to disambiguate the scale parameter in initializeChildFrame().
> scale_factor is quite ambiguous and IMO that's led to confusion here, please
> name it device_scale_factor so future generations will know what we're talking
> about. Also, in your issue description 's/page scale factor/device scale
> factor'.

Done. I'll follow with another patch to rename it in content.

[bokan, 2015-09-08 20:06:03]

lgtm (nit: issue description still says page scale)

[lfg, 2015-09-08 20:25:48]

+sky FYI, since this changes the interface used by HTMLFrame.

[dcheng, 2015-09-08 21:35:04]

lgtm

[lfg, 2015-09-08 21:35:17]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-09-08 21:35:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312903017/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312903017/40001

[commit-bot: I haz the power, 2015-09-08 21:41:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 21:41:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lfg, 2015-09-08 21:45:40]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-09-08 21:45:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1312903017/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-08 21:45:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312903017/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312903017/60001

[commit-bot: I haz the power, 2015-09-08 23:24:55]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201936

[sky, 2015-09-08 23:57:40]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-09-08 23:57:41]

I'm hoping this doesn't impact htmlviewer. Will find out shortly though.

[blink-reviews, 2015-09-09 00:38:55]

It shouldn't, HTMLFrame ignores that parameter and gets the scale factor
from the global_state(). Not sure if that's the best though or if that
should be refactored in htmlviewer.


On Tue, Sep 8, 2015 at 7:57 PM <sky@chromium.org> wrote:

> I'm hoping this doesn't impact htmlviewer. Will find out shortly though.
>
> https://codereview.chromium.org/1312903017/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sky, 2015-09-09 15:09:51]

All frames in a tree should have the same scale factor, right?

On Tue, Sep 8, 2015 at 5:38 PM, Lucas Gadani <lfg@google.com> wrote:
> It shouldn't, HTMLFrame ignores that parameter and gets the scale factor
> from the global_state(). Not sure if that's the best though or if that
> should be refactored in htmlviewer.
>
>
> On Tue, Sep 8, 2015 at 7:57 PM <sky@chromium.org> wrote:
>>
>> I'm hoping this doesn't impact htmlviewer. Will find out shortly though.
>>
>> https://codereview.chromium.org/1312903017/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[bokan, 2015-09-09 15:51:29]

On 2015/09/09 15:09:51, sky wrote:
> All frames in a tree should have the same scale factor, right?
> 
Yes, both device scale factor and page scale factor should be the same. In the
pre-OOPIF world, they were properties of the Page (so all Frames) but I'm not
sure how that's going to change.