[Webkit-unassigned] [Bug 102054] [EFL][WK2] Add APIs to get/set whether scripts can open new windows.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102054





--- Comment #10 from Christophe Dumez <christophe.dumez at intel.com>  2012-11-15 04:03:09 PST ---
(From update of attachment 174331)
View in context: https://bugs.webkit.org/attachment.cgi?id=174331&action=review

>>>>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:323
>>>>> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
>>>> 
>>>> I think we don't need to set if enable value is same as current value.
>>>> 
>>>> For example, you may add this condition.
>>>> 
>>>> bool currentValue = settings->preferences()->javaScriptCanOpenWindowsAutomatically();
>>>> if (currentValue == enable)
>>>>     return;
>>> 
>>> I agree to your idea.
>>> But, other APIs to set enable value do not check above condition.
>>> Above condition must be added in this function?
>> 
>> I think such if check would make sense if the operation was expensive. However, these are settings and the behavior is usually to set a boolean flag in memory. In practice, adding the if check may make it slower for no good reason.
>> 
>> See for e.g.:
>> 
>> void Settings::setJavaScriptCanOpenWindowsAutomatically(bool javaScriptCanOpenWindowsAutomatically)
>> {
>>     m_javaScriptCanOpenWindowsAutomatically = javaScriptCanOpenWindowsAutomatically;
>> }
>> 
>> Would it really make sense to do if (m_javaScriptCanOpenWindowsAutomatically != javaScriptCanOpenWindowsAutomatically) m_javaScriptCanOpenWindowsAutomatically = javaScriptCanOpenWindowsAutomatically; ?
> 
> When we add this check, I also think there is no big performance gain because this is not expensive operation. In addition, to add this condition is not big operation. However, I think it doesn't generally make sense we set same value again. If the settings operation become a expensive stuff, I think this checking will prevent to waste our resource in future.
> 
> Honestly, now there is no gain by this checking. But, if we add additional operation in ewk_settings_xxx function as below,
> 
> Eina_Bool ewk_settings_encoding_detector_enabled_set(Ewk_Settings* settings, Eina_Bool enable)
> {
>     EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
> 
>     settings->preferences()->setUsesEncodingDetector(enable);
> 
>     // To do something.
> 
>     return true;
> }
> 
> In this case, we have to add this checking. But, I think it would be not bad if we add this now in order to prevent to do unnecessary operations in future.

Yes, I agree with you that if it does something more that setting a flag, adding a change check is worth it.
However, it is not the case at the moment, and I would like to avoid refactoring which adds more code (and potentially decreases performance) unless we really need to. If more logic is added later which may take time, then we can optimize then, by adding the change check as you suggest. I would hold off on it until then though.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.