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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Yuni Jeong]

Added setting APIs make it possible to allow or prevent scripts from opening the new windows automatically.

[Yuni Jeong]

Created attachment 173856 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 173856 [details]
Patch

Looks good to me, with an English nitpick: "open the new windows" -> "open new windows".

[Yuni Jeong]

Created attachment 174331 [details]
Patch

[Yuni Jeong]

I changed: "open the new windows" -> "open new windows".

[Gyuyoung Kim]

Comment on attachment 174331 [details]
Patch

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;

[Yuni Jeong]

Comment on attachment 174331 [details]
Patch

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?

[Gyuyoung Kim]

(In reply to comment #6)

> 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 so. We don't need to set even though current setting is set as parameter. I think other settings functions need to have this checking condition.

[Chris Dumez]

Comment on attachment 174331 [details]
Patch

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; ?

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 174331 [details])
> 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.

[Chris Dumez]

Comment on attachment 174331 [details]
Patch

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.

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 174331 [details] [details])
> > 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.

I would not put it as a rule to be done dogmatically every time, and introduce the check only in cases when we really have an expensive operation.

[Gyuyoung Kim]

(In reply to comment #10)
 
> 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.

This is timing problem. Ok, let's do that.

[WebKit Review Bot]

Comment on attachment 174331 [details]
Patch

Clearing flags on attachment: 174331

Committed r134760: <http://trac.webkit.org/changeset/134760>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.