Bug 110298 – [EFL][QT][WK2] Turn on ApplyDeviceScaleFactorInCompositor always.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

We turn on settings not related to useFixedLayout in CoordinatedLayerTreeHost.
1. setScrollingCoordinatorEnabled
2. setApplyDeviceScaleFactorInCompositor
3. setApplyPageScaleFactorInCompositor

[Dongseong Hwang]

Created attachment 189233 [details]
Patch

[Dongseong Hwang]

Created attachment 190201 [details]
Patch

[EFL EWS Bot]

Comment on attachment 190201 [details]
Patch

Attachment 190201 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16744776

[Dongseong Hwang]

Created attachment 190208 [details]
Patch

[Dongseong Hwang]

Created attachment 190679 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 190679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190679&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:457
>  {
>      page()->setIntrinsicDeviceScaleFactor(scale);
> +    setSize(m_size);
>  }

It is not obvious that this actually changes the size.

I would definitely add a comment. // Update internal viewport size after device-scale change.

[Dongseong Hwang]

Created attachment 191149 [details]
Patch for landing

[Dongseong Hwang]

(In reply to comment #6)
> (From update of attachment 190679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190679&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:457
> >  {
> >      page()->setIntrinsicDeviceScaleFactor(scale);
> > +    setSize(m_size);
> >  }
> 
> It is not obvious that this actually changes the size.
> 
> I would definitely add a comment. // Update internal viewport size after device-scale change.

Yes, absolutely. Thanks for review :)

[WebKit Review Bot]

Comment on attachment 191149 [details]
Patch for landing

Clearing flags on attachment: 191149

Committed r144585: <http://trac.webkit.org/changeset/144585>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

I forgot to say that this should be looked over by a WK2 owner :-(

[Dongseong Hwang]

(In reply to comment #11)
> I forgot to say that this should be looked over by a WK2 owner :-(

Oops. I forgot too. It's Monday morning. I forget how I work... I'm sorry.

How can I resolve this conflict? First of all, I confess this to one of WK2 owners.

[Benjamin Poulain]

Kenneth, please do not directly r+ patches for WebKit2.

[Dongseong Hwang]

(In reply to comment #13)
> Kenneth, please do not directly r+ patches for WebKit2.

I'm sorry. It is my fault. I'll keep the rule from now.
Benjamin, could you advice how I resolve it? rollout and re-land?
And could you sign off this patch?

[Benjamin Poulain]

> Benjamin, could you advice how I resolve it? rollout and re-land?

No sure :)

I don't know anything about the plans regarding DeviceScaleFactorInCompositor so I cannot really sign off on this.

(But I think the acronym dip should have been avoided, following our practice of not using acronyms unless they are very widely used e.g. HTTP, FTP, etc).

[Dongseong Hwang]

(In reply to comment #15)
> > Benjamin, could you advice how I resolve it? rollout and re-land?
> 
> No sure :)
Thanks for kind replying :)

> I don't know anything about the plans regarding DeviceScaleFactorInCompositor so I cannot really sign off on this.

yes. I'm waiting for smfr and andersca's review.
Currently, EFL and Qt don't query applyDeviceScaleFactorInCompositor() while setting setApplyDeviceScaleFactorInCompositor to true, because EFL and Qt don't have code related to applyDeviceScaleFactorInCompositor being false.

> (But I think the acronym dip should have been avoided, following our practice of not using acronyms unless they are very widely used e.g. HTTP, FTP, etc).
aha, I'll avoid acronym. I assumed dip is acceptable because our wiki use dip. http://trac.webkit.org/wiki/ScalesAndZooms

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> Kenneth, please do not directly r+ patches for WebKit2.

Yeah it was a mistake. I first realized that I actually r+ed if when it got landed. Sorry about that :-)