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.
Created attachment 105293 [details] Proposed Patch
CC'ing Kenneth. Hello Kenneth, could you please review this patch ? This patch adjust Bug 54312's patch into EFL port as well.
CC'ing Darin.
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.
(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.
Created attachment 105600 [details] Modified Patch
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 #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.
Created attachment 105736 [details] Modified Patch
Created attachment 105759 [details] Modified Patch Remove parameter in ewk_settings_default_timer_interval_get() because the API doesn't need parameter.
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.
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 ?
(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.
(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 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.
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 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+.
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 on attachment 106070 [details] Modified Patch Clearing flags on attachment: 106070 Committed r94380: <http://trac.webkit.org/changeset/94380>
All reviewed patches have been landed. Closing bug.