Bug 110298

Summary: [EFL][QT][WK2] Turn on ApplyDeviceScaleFactorInCompositor always.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cmarcelo, gyuyoung.kim, jturcotte, kenneth, luiz, mikhail.pozdnyakov, noam, rakuco, simon.fraser, webkit.review.bot, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 110066, 110197, 110450, 110847, 111056    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
kenneth: review+, kenneth: commit-queue-
Patch for landing none

Description Dongseong Hwang 2013-02-19 21:21:09 PST
We turn on settings not related to useFixedLayout in CoordinatedLayerTreeHost.
1. setScrollingCoordinatorEnabled
2. setApplyDeviceScaleFactorInCompositor
3. setApplyPageScaleFactorInCompositor
Comment 1 Dongseong Hwang 2013-02-19 21:22:48 PST
Created attachment 189233 [details]
Patch
Comment 2 Dongseong Hwang 2013-02-25 21:54:47 PST
Created attachment 190201 [details]
Patch
Comment 3 EFL EWS Bot 2013-02-25 22:02:57 PST
Comment on attachment 190201 [details]
Patch

Attachment 190201 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16744776
Comment 4 Dongseong Hwang 2013-02-25 22:40:11 PST
Created attachment 190208 [details]
Patch
Comment 5 Dongseong Hwang 2013-02-28 01:41:13 PST
Created attachment 190679 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2013-03-02 08:02:27 PST
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.
Comment 7 Dongseong Hwang 2013-03-03 14:53:54 PST
Created attachment 191149 [details]
Patch for landing
Comment 8 Dongseong Hwang 2013-03-03 14:54:38 PST
(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 :)
Comment 9 WebKit Review Bot 2013-03-03 15:56:13 PST
Comment on attachment 191149 [details]
Patch for landing

Clearing flags on attachment: 191149

Committed r144585: <http://trac.webkit.org/changeset/144585>
Comment 10 WebKit Review Bot 2013-03-03 15:56:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Kenneth Rohde Christiansen 2013-03-03 16:01:24 PST
I forgot to say that this should be looked over by a WK2 owner :-(
Comment 12 Dongseong Hwang 2013-03-03 17:05:42 PST
(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.
Comment 13 Benjamin Poulain 2013-03-03 17:41:20 PST
Kenneth, please do not directly r+ patches for WebKit2.
Comment 14 Dongseong Hwang 2013-03-03 17:58:10 PST
(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?
Comment 15 Benjamin Poulain 2013-03-03 18:12:42 PST
> 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).
Comment 16 Dongseong Hwang 2013-03-03 18:46:01 PST
(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
Comment 17 Kenneth Rohde Christiansen 2013-03-04 00:54:22 PST
(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 :-)