Bug 139984 – [EFL] The size of contents wasn't updated immediately after r176631

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit EFL

[Hunseop Jeong]

You can watch this problem after clicking the button of maximize window in MiniBrowser.
It looks like page has wrong scale factor.
I added logs in PageViewportController::updateMinimumScaleToFit() to fix this issue.

Logs are : (when click the maximize button)
[PageViewportController::updateMinimumScaleToFit] m_viewportSize width:1920.000000 height:964.000000
[PageViewportController::updateMinimumScaleToFit] m_contentsSize width:800.000000 height:560.000000
[PageViewportController::updateMinimumScaleToFit] currentlyScaledToFit:1
[PageViewportController::updateMinimumScaleToFit] minimumScale:2.400000
[PageViewportController::updateMinimumScaleToFit] m_minimumScaleToFit:1.000000
[PageViewportController::applyScaleAfterRenderingContents] scale:2.400000

minimumScale updated the wrong value because the size of viewport was not the same with contents.

[Hunseop Jeong]

Created attachment 243791 [details]
Patch

[Hunseop Jeong]

I try to update the size of contents immediately to fix this problem but it still occurred at www.naver.com.
I will try to find the cause to fix this issue.

[Hunseop Jeong]

Created attachment 243818 [details]
Patch

[Hunseop Jeong]

After attachment 243818 [details], Page was scaled correctly when clicked the maxmize button in MiniBrowser. 

Gyuyoung, Could I ask why you removed m_hadUserInteraction in updateMinimumScaleToFit()?

But api test still failed in EWK2ViewTest.ewk_view_scale_with_fixed_layout.

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:917: Failure
Value of: ewk_view_scale_get(webView())
  Actual: 0.81632656
Expected: 1

916:     // Default scale value is 1.0.
917:     ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));

[Gyuyoung Kim]

(In reply to comment #4)
> After attachment 243818 [details], Page was scaled correctly when clicked
> the maxmize button in MiniBrowser. 
> 
> Gyuyoung, Could I ask why you removed m_hadUserInteraction in
> updateMinimumScaleToFit()?

IIRC, I removed it because it seems we don't need to use it. If we need to use it, we can revert it.


> But api test still failed in EWK2ViewTest.ewk_view_scale_with_fixed_layout.
> 
> ../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:917: Failure
> Value of: ewk_view_scale_get(webView())
>   Actual: 0.81632656
> Expected: 1
> 
> 916:     // Default scale value is 1.0.
> 917:     ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));

Hmm, this test was passed when I landed r176631. I guess following patch made this regression.

[Hunseop Jeong]

Created attachment 244432 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 244432 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));

If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?

ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));

This is to test that webview can keep previous scale factor after reloading a page.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-924
> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.5, 0, 0));

ditto.

[Hunseop Jeong]

Comment on attachment 244432 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
>> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));
> 
> If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?
> 
> ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));

If load the defaultTestPage, pageScaleFactor was changed from 1.0 to 0.81632656 because default contentSize(980x735) bigger then default viewSize(800x560).
So I removed the "loadUrlSync(environment->defaultTestPageUrl())" to check the default value of pageScaleFactor whether it is 1.0.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
>> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));
> 
> This is to test that webview can keep previous scale factor after reloading a page.

If ewk_view_scale_set() was invoked before loadUrl(), it didn't set pageScaleFactor because syncVisibleContents() failed.
If syncVisibleContents() failed, it recover the original pageScaleFactor that was old one. 
So I moved the ewk_view_scale_set() after invoking the loadUrlSync().

[Gyuyoung Kim]

Comment on attachment 244432 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
>>> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));
>> 
>> If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?
>> 
>> ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));
> 
> If load the defaultTestPage, pageScaleFactor was changed from 1.0 to 0.81632656 because default contentSize(980x735) bigger then default viewSize(800x560).
> So I removed the "loadUrlSync(environment->defaultTestPageUrl())" to check the default value of pageScaleFactor whether it is 1.0.

If so, we have to change the default content size with 800x560. I think you change test behavior without reasonable reason.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
>>> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));
>> 
>> This is to test that webview can keep previous scale factor after reloading a page.
> 
> If ewk_view_scale_set() was invoked before loadUrl(), it didn't set pageScaleFactor because syncVisibleContents() failed.
> If syncVisibleContents() failed, it recover the original pageScaleFactor that was old one. 
> So I moved the ewk_view_scale_set() after invoking the loadUrlSync().

I don't understand well. We already load a default test page in 914 line before you removed it. If so, ewk_view_scale_set(webView(), 1.2, 0, 0) should not be failed, isn't it ?

[Hunseop Jeong]

Comment on attachment 244432 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
>>>> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));
>>> 
>>> If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?
>>> 
>>> ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));
>> 
>> If load the defaultTestPage, pageScaleFactor was changed from 1.0 to 0.81632656 because default contentSize(980x735) bigger then default viewSize(800x560).
>> So I removed the "loadUrlSync(environment->defaultTestPageUrl())" to check the default value of pageScaleFactor whether it is 1.0.
> 
> If so, we have to change the default content size with 800x560. I think you change test behavior without reasonable reason.

Yes, ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView())) passed if I changed the default page width from 980 to 800 in ViewportConfiguration::webpageParameters().
Should I changed the value in EFL port?

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
>>>> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));
>>> 
>>> This is to test that webview can keep previous scale factor after reloading a page.
>> 
>> If ewk_view_scale_set() was invoked before loadUrl(), it didn't set pageScaleFactor because syncVisibleContents() failed.
>> If syncVisibleContents() failed, it recover the original pageScaleFactor that was old one. 
>> So I moved the ewk_view_scale_set() after invoking the loadUrlSync().
> 
> I don't understand well. We already load a default test page in 914 line before you removed it. If so, ewk_view_scale_set(webView(), 1.2, 0, 0) should not be failed, isn't it ?

Yes, it is. But it failed if I changed the default page width to 800. 

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:922: Failure
Value of: ewk_view_scale_get(webView())
  Actual: 0.81632656
Expected: 1.2

I think ewk_view_scale_set(webView(), 1.2, 0, 0) didn'set the pageScaleFactor because syncVisibleContents() failed.
So it recover the old pageScaleFactor but I don't know why actual value is 0.81632656.

[Gyuyoung Kim]

Comment on attachment 244432 [details]
Patch

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

>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
>>>>> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));
>>>> 
>>>> If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?
>>>> 
>>>> ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));
>>> 
>>> If load the defaultTestPage, pageScaleFactor was changed from 1.0 to 0.81632656 because default contentSize(980x735) bigger then default viewSize(800x560).
>>> So I removed the "loadUrlSync(environment->defaultTestPageUrl())" to check the default value of pageScaleFactor whether it is 1.0.
>> 
>> If so, we have to change the default content size with 800x560. I think you change test behavior without reasonable reason.
> 
> Yes, ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView())) passed if I changed the default page width from 980 to 800 in ViewportConfiguration::webpageParameters().
> Should I changed the value in EFL port?

When I landed r176631, this API test was passed. Could you check if content Size was also 980x735 at that time ?

>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
>>>>> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));
>>>> 
>>>> This is to test that webview can keep previous scale factor after reloading a page.
>>> 
>>> If ewk_view_scale_set() was invoked before loadUrl(), it didn't set pageScaleFactor because syncVisibleContents() failed.
>>> If syncVisibleContents() failed, it recover the original pageScaleFactor that was old one. 
>>> So I moved the ewk_view_scale_set() after invoking the loadUrlSync().
>> 
>> I don't understand well. We already load a default test page in 914 line before you removed it. If so, ewk_view_scale_set(webView(), 1.2, 0, 0) should not be failed, isn't it ?
> 
> Yes, it is. But it failed if I changed the default page width to 800. 
> 
> ../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:922: Failure
> Value of: ewk_view_scale_get(webView())
>   Actual: 0.81632656
> Expected: 1.2
> 
> I think ewk_view_scale_set(webView(), 1.2, 0, 0) didn'set the pageScaleFactor because syncVisibleContents() failed.
> So it recover the old pageScaleFactor but I don't know why actual value is 0.81632656.

I made to restore previous scale factor if sysnVisibleContents() is failed. I think we should not keep the failing pageScaleFactor in PageViewportController though coordinatedGraphics fails to change scale. I think we should check why syncVisibleContents() is failed first.

[Hunseop Jeong]

Comment on attachment 244432 [details]
Patch

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

>>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-914
>>>>>> -    ASSERT_TRUE(loadUrlSync(environment->defaultTestPageUrl()));
>>>>> 
>>>>> If we don't load a test page, what is meaning to check if default scale value is 1.0 below ?
>>>>> 
>>>>> ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView()));
>>>> 
>>>> If load the defaultTestPage, pageScaleFactor was changed from 1.0 to 0.81632656 because default contentSize(980x735) bigger then default viewSize(800x560).
>>>> So I removed the "loadUrlSync(environment->defaultTestPageUrl())" to check the default value of pageScaleFactor whether it is 1.0.
>>> 
>>> If so, we have to change the default content size with 800x560. I think you change test behavior without reasonable reason.
>> 
>> Yes, ASSERT_FLOAT_EQ(1, ewk_view_scale_get(webView())) passed if I changed the default page width from 980 to 800 in ViewportConfiguration::webpageParameters().
>> Should I changed the value in EFL port?
> 
> When I landed r176631, this API test was passed. Could you check if content Size was also 980x735 at that time ?

I tried to test the ewk_view_scale_with_fixed_layout after reset r176631.
Failed in 5 times among the 10 times.
Maybe,,there are some timing issue.

Value of: ewk_view_scale_get(webView())
  Actual: 0.81632656
Expected: 1.5

Value of: ewk_view_scale_get(webView())
  Actual: 0.81632656
Expected: 1.2

Result is different whenever I try to test.

And I checked the content size in PageViewportController::updateMinimumScaleToFit().

contentSize width:980.000000 height:735.000000
m_viewportSize width:800.000000 height:600.000000
minimumScale:0.816327

>>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-919
>>>>>> -    ASSERT_TRUE(ewk_view_scale_set(webView(), 1.2, 0, 0));
>>>>> 
>>>>> This is to test that webview can keep previous scale factor after reloading a page.
>>>> 
>>>> If ewk_view_scale_set() was invoked before loadUrl(), it didn't set pageScaleFactor because syncVisibleContents() failed.
>>>> If syncVisibleContents() failed, it recover the original pageScaleFactor that was old one. 
>>>> So I moved the ewk_view_scale_set() after invoking the loadUrlSync().
>>> 
>>> I don't understand well. We already load a default test page in 914 line before you removed it. If so, ewk_view_scale_set(webView(), 1.2, 0, 0) should not be failed, isn't it ?
>> 
>> Yes, it is. But it failed if I changed the default page width to 800. 
>> 
>> ../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:922: Failure
>> Value of: ewk_view_scale_get(webView())
>>   Actual: 0.81632656
>> Expected: 1.2
>> 
>> I think ewk_view_scale_set(webView(), 1.2, 0, 0) didn'set the pageScaleFactor because syncVisibleContents() failed.
>> So it recover the old pageScaleFactor but I don't know why actual value is 0.81632656.
> 
> I made to restore previous scale factor if sysnVisibleContents() is failed. I think we should not keep the failing pageScaleFactor in PageViewportController though coordinatedGraphics fails to change scale. I think we should check why syncVisibleContents() is failed first.

I checked the syncVisiblieContents() whether is failed or not.
It is not a problem of syncVisibleContent(),,,
syncVisibleContents() isn't failed in test.
I will find what is problem.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.