Bug 67012 - [EFL] Allow controlling minimum DOMTimer interval on a per-page basis
Summary: [EFL] Allow controlling minimum DOMTimer interval on a per-page basis
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 54312
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-25 19:19 PDT by Gyuyoung Kim
Modified: 2011-09-01 18:34 PDT (History)
7 users (show)

See Also:


Attachments
Proposed Patch (4.10 KB, patch)
2011-08-25 19:26 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (5.98 KB, patch)
2011-08-30 02:15 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (6.38 KB, patch)
2011-08-30 21:28 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (6.35 KB, patch)
2011-08-31 01:25 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (6.34 KB, patch)
2011-08-31 20:15 PDT, Gyuyoung Kim
kbr: review+
Details | Formatted Diff | Diff
Modified Patch (6.28 KB, patch)
2011-09-01 17:33 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-08-25 19:19:20 PDT
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.
Comment 1 Gyuyoung Kim 2011-08-25 19:26:18 PDT
Created attachment 105293 [details]
Proposed Patch
Comment 2 Gyuyoung Kim 2011-08-25 19:28:27 PDT
CC'ing Kenneth. Hello Kenneth, could you please review this patch ? This patch adjust Bug 54312's patch into EFL port as well.
Comment 3 Gyuyoung Kim 2011-08-26 01:01:07 PDT
CC'ing Darin.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-08-26 05:50:15 PDT
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.
Comment 5 Gyuyoung Kim 2011-08-30 02:14:36 PDT
(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.
Comment 6 Gyuyoung Kim 2011-08-30 02:15:52 PDT
Created attachment 105600 [details]
Modified Patch
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-08-30 07:08:19 PDT
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.
Comment 8 Gyuyoung Kim 2011-08-30 21:27:53 PDT
(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.
Comment 9 Gyuyoung Kim 2011-08-30 21:28:41 PDT
Created attachment 105736 [details]
Modified Patch
Comment 10 Gyuyoung Kim 2011-08-31 01:25:48 PDT
Created attachment 105759 [details]
Modified Patch

Remove parameter in ewk_settings_default_timer_interval_get() because the API doesn't need parameter.
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-08-31 09:21:35 PDT
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.
Comment 12 Gyuyoung Kim 2011-08-31 20:15:02 PDT
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 ?
Comment 13 Gyuyoung Kim 2011-09-01 00:00:09 PDT
(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.
Comment 14 Grzegorz Czajkowski 2011-09-01 00:14:37 PDT
(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.
Comment 15 Kenneth Russell 2011-09-01 12:51:03 PDT
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.
Comment 16 Raphael Kubo da Costa (:rakuco) 2011-09-01 14:12:17 PDT
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.
Comment 17 Kenneth Russell 2011-09-01 15:00:36 PDT
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+.
Comment 18 Gyuyoung Kim 2011-09-01 17:33:55 PDT
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.
Comment 19 WebKit Review Bot 2011-09-01 18:34:19 PDT
Comment on attachment 106070 [details]
Modified Patch

Clearing flags on attachment: 106070

Committed r94380: <http://trac.webkit.org/changeset/94380>
Comment 20 WebKit Review Bot 2011-09-01 18:34:24 PDT
All reviewed patches have been landed.  Closing bug.