Bug 114947 - [EFL] Prevent to apply a large scale factor on launching the MiniBrowser.
Summary: [EFL] Prevent to apply a large scale factor on launching the MiniBrowser.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: JungJik Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-22 04:31 PDT by JungJik Lee
Modified: 2017-03-11 10:45 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2013-04-22 05:15 PDT, JungJik Lee
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2013-04-23 04:37 PDT, JungJik Lee
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2013-05-07 18:47 PDT, JungJik Lee
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2013-05-07 23:13 PDT, JungJik Lee
no flags Details | Formatted Diff | Diff
Patch (3.13 KB, patch)
2013-05-15 21:13 PDT, JungJik Lee
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (754.60 KB, application/zip)
2013-05-15 23:38 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description JungJik Lee 2013-04-22 04:31:13 PDT
when we launch the MiniBrowser, sometimes the browser is stopped for a while. It is owing to a large scale factor. the ewk_view size is 128x5(w,h) at initial time and to fit to web page to view size, the browser applies a large scale factor in updateMinimumScaleToFit function up to scale 70.0f. On the other hand WebProcess is parsing the page, so when the scale factor is applied to backing store,  the actual page size becomes 800x600.  this makes the TBS insanely big (800 width * 70.0f).
the browser is stopped for drawing all tiles with that size. So what makes the view size grow? because we use EVAS_HINT_EXPAND in ewk_view. this flag makes the evas increase the size as time goes by. we should limit the hint size to fix size evas.
evas_object_size_hint_min_set function give a limitation to HINT_EXPAND.
Comment 1 JungJik Lee 2013-04-22 05:15:06 PDT
Created attachment 199015 [details]
Patch
Comment 2 JungJik Lee 2013-04-23 04:37:45 PDT
Created attachment 199215 [details]
Patch
Comment 3 Gyuyoung Kim 2013-04-24 18:05:47 PDT
CC'ing graphics folks.
Comment 4 Gyuyoung Kim 2013-04-30 03:29:09 PDT
Comment on attachment 199215 [details]
Patch

It looks this patch makes sense.
Comment 5 Ryuan Choi 2013-05-01 20:06:52 PDT
Comment on attachment 199215 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:1566
> +    evas_object_size_hint_min_set(window->ewk_view, width ? width : window_width, height ? height : window_height);

Could you try after moved evas_object_resize (at 1574 line) here ?

This code added wrong behavior which does not reduce webview size when resized window smaller.
Comment 6 JungJik Lee 2013-05-01 21:36:00 PDT
(In reply to comment #5)
> (From update of attachment 199215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199215&action=review
> 
> > Tools/MiniBrowser/efl/main.c:1566
> > +    evas_object_size_hint_min_set(window->ewk_view, width ? width : window_width, height ? height : window_height);
> 
> Could you try after moved evas_object_resize (at 1574 line) here ?
> 
> This code added wrong behavior which does not reduce webview size when resized window smaller.

Yes, I know that. so I asked not to commit this patch to gyuyoung until I check that you mentioned. I'll check it again. thanks your comment.
Comment 7 JungJik Lee 2013-05-07 18:47:29 PDT
Created attachment 201013 [details]
Patch
Comment 8 JungJik Lee 2013-05-07 19:32:58 PDT
I explain more about this issue.
The issue is that when we launch the MiniBrowser, the MiniBrowser is paused for 1 ~ 3 seconds often. It's a timing issue. this occurs only in EFL.

The cause is that we use elm_box for layout and the layouting is not finished at one go. Due to the other child boxes _els_box_layout(EFL function) is called three times for the layout calculation. As a result, the ewk_view also is resized like this.

EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=127, height=0)
EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=250, height=25)
EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=800, height=564)

And the actual problem is when ewk_view size is 127,0, UIProcess calls PageViewportController::updateMinimumScaleToFit and send the result scale factor (70.5f) to WebProcess. When WebProcess receives the result, TBS has a normal  viewport size layer. TBS creates tiles as large as multiplying by scale 800(width)x70.5(scale) and draws the tiles. That makes pausing.

I checked ryuan.choi's comment. Even we change the position of evas_object_resize, it is possible to be remained.
Comment 9 Gyuyoung Kim 2013-05-07 22:35:23 PDT
Comment on attachment 201013 [details]
Patch

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

> Tools/ChangeLog:6
> +        Reviewed by Gyuyoung Kim.

Please remove my name. This patch needs to be reviewed again.
Comment 10 Mikhail Pozdnyakov 2013-05-07 22:39:42 PDT
Comment on attachment 201013 [details]
Patch

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

looks like a work-around, might be ok thought

> Tools/ChangeLog:9
> +        resized from small size to default size gradually. This causes a large

what do you mean by "gradually"? web view size is set multiple times?

> Tools/ChangeLog:10
> +        scale factor in updateMinimumScaleToFit function. And the large scale

could you please point the class name also? PageViewportController::updateMinimumScaleToFit?

> Tools/ChangeLog:11
> +        factor is applied to TBS. As a result, the MiniBrowser is stopped for

TBS? Please avoid using acronyms in change log.

> Tools/MiniBrowser/efl/main.c:59
> +static Eina_Bool ignore_initial_layout = EINA_FALSE;

can be static inside the function.

> Tools/MiniBrowser/efl/main.c:418
> +on_resize(void *user_data, Evas *e, Evas_Object *ewk_view, void *event_info)

variables that are not used can be omitted.
Comment 11 Ryuan Choi 2013-05-07 23:02:48 PDT
(In reply to comment #8)
> I explain more about this issue.
> The issue is that when we launch the MiniBrowser, the MiniBrowser is paused for 1 ~ 3 seconds often. It's a timing issue. this occurs only in EFL.
> 
> The cause is that we use elm_box for layout and the layouting is not finished at one go. Due to the other child boxes _els_box_layout(EFL function) is called three times for the layout calculation. As a result, the ewk_view also is resized like this.
> 
> EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=127, height=0)
> EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=250, height=25)
> EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=800, height=564)
> 
> And the actual problem is when ewk_view size is 127,0, UIProcess calls PageViewportController::updateMinimumScaleToFit and send the result scale factor (70.5f) to WebProcess. When WebProcess receives the result, TBS has a normal  viewport size layer. TBS creates tiles as large as multiplying by scale 800(width)x70.5(scale) and draws the tiles. That makes pausing.
> 
> I checked ryuan.choi's comment. Even we change the position of evas_object_resize, it is possible to be remained.

Doesn't we need to handle exception instead of just adding workaround for minibrowser?
Anybody might mistake to resize ewk view to zero width or height.

What do you think about it?
Comment 12 JungJik Lee 2013-05-07 23:02:58 PDT
Comment on attachment 201013 [details]
Patch

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

>> Tools/ChangeLog:6
>> +        Reviewed by Gyuyoung Kim.
> 
> Please remove my name. This patch needs to be reviewed again.

Okay. I will.

>> Tools/ChangeLog:9
>> +        resized from small size to default size gradually. This causes a large
> 
> what do you mean by "gradually"? web view size is set multiple times?

yes, multiple times.

>> Tools/ChangeLog:10
>> +        scale factor in updateMinimumScaleToFit function. And the large scale
> 
> could you please point the class name also? PageViewportController::updateMinimumScaleToFit?

okay.

>> Tools/ChangeLog:11
>> +        factor is applied to TBS. As a result, the MiniBrowser is stopped for
> 
> TBS? Please avoid using acronyms in change log.

It means TiledBackingStore. Okay,I'll write a full name.

>> Tools/MiniBrowser/efl/main.c:59
>> +static Eina_Bool ignore_initial_layout = EINA_FALSE;
> 
> can be static inside the function.

Okay. I'll move it.

>> Tools/MiniBrowser/efl/main.c:418
>> +on_resize(void *user_data, Evas *e, Evas_Object *ewk_view, void *event_info)
> 
> variables that are not used can be omitted.

if we remove the parameter name, compiler shows this log > error: parameter name omitted.
Comment 13 JungJik Lee 2013-05-07 23:13:24 PDT
Created attachment 201031 [details]
Patch
Comment 14 JungJik Lee 2013-05-07 23:16:50 PDT
(In reply to comment #10)
> (From update of attachment 201013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201013&action=review
> 
> looks like a work-around, might be ok thought
> 
Yes, it looks like. but I couldn't find the best way to avoid this situation.
Thanks your comments.

> > Tools/ChangeLog:9
> > +        resized from small size to default size gradually. This causes a large
> 
> what do you mean by "gradually"? web view size is set multiple times?
> 
> > Tools/ChangeLog:10
> > +        scale factor in updateMinimumScaleToFit function. And the large scale
> 
> could you please point the class name also? PageViewportController::updateMinimumScaleToFit?
> 
> > Tools/ChangeLog:11
> > +        factor is applied to TBS. As a result, the MiniBrowser is stopped for
> 
> TBS? Please avoid using acronyms in change log.
> 
> > Tools/MiniBrowser/efl/main.c:59
> > +static Eina_Bool ignore_initial_layout = EINA_FALSE;
> 
> can be static inside the function.
> 
> > Tools/MiniBrowser/efl/main.c:418
> > +on_resize(void *user_data, Evas *e, Evas_Object *ewk_view, void *event_info)
> 
> variables that are not used can be omitted.
Comment 15 JungJik Lee 2013-05-08 07:33:57 PDT
(In reply to comment #11)
> (In reply to comment #8)
> > I explain more about this issue.
> > The issue is that when we launch the MiniBrowser, the MiniBrowser is paused for 1 ~ 3 seconds often. It's a timing issue. this occurs only in EFL.
> > 
> > The cause is that we use elm_box for layout and the layouting is not finished at one go. Due to the other child boxes _els_box_layout(EFL function) is called three times for the layout calculation. As a result, the ewk_view also is resized like this.
> > 
> > EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=127, height=0)
> > EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=250, height=25)
> > EwkView::handleEvasObjectResize (evasObject=0x9b9370, width=800, height=564)
> > 
> > And the actual problem is when ewk_view size is 127,0, UIProcess calls PageViewportController::updateMinimumScaleToFit and send the result scale factor (70.5f) to WebProcess. When WebProcess receives the result, TBS has a normal  viewport size layer. TBS creates tiles as large as multiplying by scale 800(width)x70.5(scale) and draws the tiles. That makes pausing.
> > 
> > I checked ryuan.choi's comment. Even we change the position of evas_object_resize, it is possible to be remained.
> 
> Doesn't we need to handle exception instead of just adding workaround for minibrowser?
> Anybody might mistake to resize ewk view to zero width or height.
> 
> What do you think about it?

I missed this comment. this issue is happened because of elm box layout structure. if we don't use the elm box layout,it wouldn't happen. so I think it's better to handle this exception in minibrowser than EwkView.cpp.
Comment 16 JungJik Lee 2013-05-15 18:59:43 PDT
the root cause of this issue is the imparity contentsSize of WebProcess and UIProcess. However it is going to remove sync message type from WK2, so I think it is better to handle this issue in MiniBrowser.

EBPROCESS virtual void WebKit::WebChromeClient::contentsSizeChanged(WebCore::Frame*, const WebCore::IntSize&) const 127 8
WEBPROCESS virtual void WebKit::WebChromeClient::contentsSizeChanged(WebCore::Frame*, const WebCore::IntSize&) const 800 564
UIPROCESS void WebKit::PageViewportController::didChangeContentsSize(const WebCore::IntSize&) 127 8
-> at this time if we calculate the scale factor, UIProcess know 127,8 size,
But at this time WebProcess 800,564.
UIPROCESS void WebKit::PageViewportController::didChangeContentsSize(const WebCore::IntSize&) 800 564
-> at this time WebProcess accept the scale factor and create a big TiledBackingStore.

check out this patch, https://bugs.webkit.org/show_bug.cgi?id=116105
Comment 17 Gyuyoung Kim 2013-05-15 19:26:44 PDT
Comment on attachment 201031 [details]
Patch

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

I also think this isn't correct way to fix this problem. However, I think this problem has blocked EFL development. So, I'd like to land this patch at the moment. Any other objection or comment ?

> Tools/MiniBrowser/efl/main.c:419
> +    static Eina_Bool ignore_initial_layout = EINA_FALSE;

Could you add a comment why this logic we need using "FIXME:" ?
Comment 18 JungJik Lee 2013-05-15 21:13:33 PDT
Created attachment 201917 [details]
Patch
Comment 19 Build Bot 2013-05-15 23:38:13 PDT
Comment on attachment 201917 [details]
Patch

Attachment 201917 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/483182

New failing tests:
fast/events/mouse-cursor-image-set.html
Comment 20 Build Bot 2013-05-15 23:38:16 PDT
Created attachment 201929 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 21 Chris Dumez 2013-05-16 08:29:58 PDT
(In reply to comment #17)
> (From update of attachment 201031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201031&action=review
> 
> I also think this isn't correct way to fix this problem. However, I think this problem has blocked EFL development. So, I'd like to land this patch at the moment. Any other objection or comment ?

Is this really blocking EFL development? MiniBrowser seems to behave acceptably for me and it does not affect my work in any way.

I agree with previous comments that this feels like a hack and we would better take time to find a proper fix instead of landing it.

If this really blocks EFL development, please elaborate.
Comment 22 Gyuyoung Kim 2013-05-16 16:13:13 PDT
(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 201031 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=201031&action=review
> > 
> > I also think this isn't correct way to fix this problem. However, I think this problem has blocked EFL development. So, I'd like to land this patch at the moment. Any other objection or comment ?
> 
> Is this really blocking EFL development? MiniBrowser seems to behave acceptably for me and it does not affect my work in any way.
> 
> I agree with previous comments that this feels like a hack and we would better take time to find a proper fix instead of landing it.

Yes, I agree that this patch is not proper fix.
 
> If this really blocks EFL development, please elaborate.

If you run MiniBrowser on Ubuntu 13.04, it is locked up frequently. I also think this should be fixed with proper solution though, we couldn't find proper fix during a month. If you think we need to find proper fix further, I don't mind it. But, if there is no good fix at the moment, please consider this patch again.
Comment 23 Gyuyoung Kim 2013-10-20 22:30:07 PDT
Comment on attachment 201917 [details]
Patch

Doesn't this problem happen anymore ? If so, please close this bug.
Comment 24 Michael Catanzaro 2017-03-11 10:45:04 PST
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.