Added setting APIs make it possible to allow or prevent scripts from opening the new windows automatically.
Created attachment 173856 [details] Patch
Comment on attachment 173856 [details] Patch Looks good to me, with an English nitpick: "open the new windows" -> "open new windows".
Created attachment 174331 [details] Patch
I changed: "open the new windows" -> "open new windows".
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;
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?
(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.
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; ?
(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.
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.
(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.
(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.
Comment on attachment 174331 [details] Patch Clearing flags on attachment: 174331 Committed r134760: <http://trac.webkit.org/changeset/134760>
All reviewed patches have been landed. Closing bug.