Bug 67012 – [EFL] Allow controlling minimum DOMTimer interval on a per-page basis

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Set default minimum DOMTimer interval to 4ms instead of 10ms. Because, 4ms is known to better minimum value for performance. Mac, android, chromium and window ports already use 4ms instead of 10ms. In addition, it is desirable to be able to change the minimum DOMTimer interval on per-page basis in order to avoid consuming excessive CPU and battery life on mobile devices. In addition, other ports have already used this setting by means of Bug 54312.

[Gyuyoung Kim]

Created attachment 105293 [details]
Proposed Patch

[Gyuyoung Kim]

CC'ing Kenneth. Hello Kenneth, could you please review this patch ? This patch adjust Bug 54312's patch into EFL port as well.

[Gyuyoung Kim]

CC'ing Darin.

[Raphael Kubo da Costa (:rakuco)]

Informal r- for now.

> Source/WebKit/efl/ewk/ewk_view.cpp:2349
> +    priv->page_settings->setMinDOMTimerInterval(interval);

The idiom we're currently using for settings is to have a variable in the settings struct in the smart data, so the interaction is first done with it (the same applies to the getter below).

> Source/WebKit/efl/ewk/ewk_view.h:1874
> + * Sets the minimum interval for DOMTimers on current page.

I think you could explain very briefly in the apidox what DOMTimers are.

> Source/WebKit/efl/ewk/ewk_view.h:1876
> + * Currently, minimum interval value is 4ms by default. If less minimum interval value is set,

The whole "If less minimum [...] function more frequently" part is hard to understand -- I couldn't get what the first sentence stood for, and the second one looks redundant.

> Source/WebKit/efl/ewk/ewk_view.h:1887
> + * Gets the default interval for DOMTimers on all pages.

Doesn't it make more sense to return the minimum interval time set for this page? If the apidox for the setter already says the default value is 4ms and you can't change it, there is little point in returning it while not returning the value for the current page.

[Gyuyoung Kim]

(In reply to comment #4)
> Informal r- for now.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:1874
> > + * Sets the minimum interval for DOMTimers on current page.
> 
> I think you could explain very briefly in the apidox what DOMTimers are.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:1876
> > + * Currently, minimum interval value is 4ms by default. If less minimum interval value is set,
> 
> The whole "If less minimum [...] function more frequently" part is hard to understand -- I couldn't get what the first sentence stood for, and the second one looks redundant.

Ok, it also seems to me second description is not needed. Other ports just also explain very briefly as below links,

https://bugs.webkit.org/attachment.cgi?id=82521&action=diff#Source/WebKit/mac/WebView/WebViewPrivate.h_sec1

https://bugs.webkit.org/attachment.cgi?id=82521&action=diff#Source/WebKit/win/Interfaces/IWebViewPrivate.idl_sec1

So, I remove redundant description.

> 
> > Source/WebKit/efl/ewk/ewk_view.h:1887
> > + * Gets the default interval for DOMTimers on all pages.
> 
> Doesn't it make more sense to return the minimum interval time set for this page? If the apidox for the setter already says the default value is 4ms and you can't change it, there is little point in returning it while not returning the value for the current page.

This patch is to control minimum interval of DOMTimer on per page basis. Because, we don't want that background tab consumes cpu, battery resource on multi tabs browsing. (This is desirable on mobile devices.) So, application only needs to know both default minimum interval for all pages and how to set the minimum interval on specific page. So, I added only two APIs. 

However, as you said, smart data struct maintains setting variables for a page.
I also think it is better to add both an interval variable in smart data struct and minimum_timer_interval_get() for a page. Nevertheless, application still needs to know the default minimum value in order to set default interval again. I move the API to ewk_settings.cpp because that is more proper location.

However, I think we don't need to change default interval. mac, window port don't support to change default timer interval yet.

[Gyuyoung Kim]

Created attachment 105600 [details]
Modified Patch

[Raphael Kubo da Costa (:rakuco)]

Looks mostly OK.

> Source/WebKit/efl/ewk/ewk_settings.h:198
> + * Gets the default interval for DOMTimers on all pages.

I still think it'd be nice to explain very briefly what DOMTimers are.

> Source/WebKit/efl/ewk/ewk_view.cpp:2378
> +    priv->page_settings->setMinDOMTimerInterval(interval);

At least for boolean values, we usually call WebCore only when the new value differs from the value in the smart data. It might be worth doing the same here.

[Gyuyoung Kim]

(In reply to comment #7)
> Looks mostly OK.
> 
> > Source/WebKit/efl/ewk/ewk_settings.h:198
> > + * Gets the default interval for DOMTimers on all pages.
> 
> I still think it'd be nice to explain very briefly what DOMTimers are.
Done.

> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:2378
> > +    priv->page_settings->setMinDOMTimerInterval(interval);
> 
> At least for boolean values, we usually call WebCore only when the new value differs from the value in the smart data. It might be worth doing the same here.

I use fabs() and epsilon() in order to solve comparison problem of "double" data type.

[Gyuyoung Kim]

Created attachment 105736 [details]
Modified Patch

[Gyuyoung Kim]

Created attachment 105759 [details]
Modified Patch

Remove parameter in ewk_settings_default_timer_interval_get() because the API doesn't need parameter.

[Raphael Kubo da Costa (:rakuco)]

r+ from my side after fixing this:


> Source/WebKit/efl/ewk/ewk_settings.cpp:282
> +double ewk_settings_default_timer_interval_get(void)

This is C++ code, so the void here should be removed.

> Source/WebKit/efl/ewk/ewk_settings.h:206
> +EAPI double           ewk_settings_default_timer_interval_get(void);

Idem.

[Gyuyoung Kim]

Created attachment 105888 [details]
Modified Patch

Ok, I fix it. 

Kenneth and Darin, 

It looks this patch passes informal review. Could you review this patch ?

[Gyuyoung Kim]

(In reply to comment #7)
> Looks mostly OK.
> 
> > Source/WebKit/efl/ewk/ewk_settings.h:198
> > + * Gets the default interval for DOMTimers on all pages.
> 
> I still think it'd be nice to explain very briefly what DOMTimers are.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:2378
> > +    priv->page_settings->setMinDOMTimerInterval(interval);
> 
> At least for boolean values, we usually call WebCore only when the new value differs from the value in the smart data. It might be worth doing the same here.

(In reply to comment #11)
> r+ from my side after fixing this:
> 
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:282
> > +double ewk_settings_default_timer_interval_get(void)
> 
> This is C++ code, so the void here should be removed.

ewk_settings.cpp is C++? As you know, ewk_xxx files just have .cpp file format. Real implementation is based on C. So, I don't know if we should adhere C++ coding style. In addition, all functions in ewk_settings.h except for one API are using *void* parameter.

[Grzegorz Czajkowski]

(In reply to comment #13)
> (In reply to comment #7)
> > Looks mostly OK.
> > 
> > > Source/WebKit/efl/ewk/ewk_settings.h:198
> > > + * Gets the default interval for DOMTimers on all pages.
> > 
> > I still think it'd be nice to explain very briefly what DOMTimers are.
> > 
> > > Source/WebKit/efl/ewk/ewk_view.cpp:2378
> > > +    priv->page_settings->setMinDOMTimerInterval(interval);
> > 
> > At least for boolean values, we usually call WebCore only when the new value differs from the value in the smart data. It might be worth doing the same here.
> 
> (In reply to comment #11)
> > r+ from my side after fixing this:
> > 
> > 
> > > Source/WebKit/efl/ewk/ewk_settings.cpp:282
> > > +double ewk_settings_default_timer_interval_get(void)
> > 
> > This is C++ code, so the void here should be removed.
> 
> ewk_settings.cpp is C++? As you know, ewk_xxx files just have .cpp file format. Real implementation is based on C. So, I don't know if we should adhere C++ coding style. In addition, all functions in ewk_settings.h except for one API are using *void* parameter.

There was a commit https://bugs.webkit.org/show_bug.cgi?id=56821 which replaces () to (void) for API. I think we should keep this convention because generally we expose API in C.

[Kenneth Russell]

Comment on attachment 105888 [details]
Modified Patch

It looks like there's still some stylistic disagreement. Please submit a new patch and/or mark r? if you want a formal review once it's passed informal review.

[Raphael Kubo da Costa (:rakuco)]

Even though I personally don't agree with the C-style changes in this patch or the committed one, I'd rather have this one get in and get over the bikeshedding, so r+ from my side again.

[Kenneth Russell]

Comment on attachment 105888 [details]
Modified Patch

OK. In the interests of moving this forward I'm marking this r+. I leave it to one of the EFL team to mark it cq+.

[Gyuyoung Kim]

Created attachment 106070 [details]
Modified Patch

Kenneth, thank you for your r+.

Kubo, if you think we have to go c++ coding style, let's discuss it on new bug. For now, I think we need to adhere c coding style.

[WebKit Review Bot]

Comment on attachment 106070 [details]
Modified Patch

Clearing flags on attachment: 106070

Committed r94380: <http://trac.webkit.org/changeset/94380>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.