Bug 58016

Created attachment 88586 [details] patch

Created attachment 88604 [details] patch ewk_setting_cache_capacity_set is added

Comment on attachment 88604 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=88604&action=review > Source/WebKit/efl/ewk/ewk_settings.cpp:352 > +Eina_Bool ewk_settings_cache_disable_set(Eina_Bool set) I am not sure, but, it seems to me it is clear to use ewk_settings_cache_enable_set() instead of ewk_settings_cache_disable_set()

Created attachment 89181 [details] reverse logic

Comment on attachment 89181 [details] reverse logic Please set cq+ when it passes any API review you guys have.

Comment on attachment 89181 [details] reverse logic View in context: https://bugs.webkit.org/attachment.cgi?id=89181&action=review > Source/WebKit/efl/ewk/ewk_settings.cpp:349 > + * @param @c EINA_TRUE to enable cache of WebCore , @c EINA_FALSE to disable @param set... > Source/WebKit/efl/ewk/ewk_settings.cpp:355 > + EINA_SAFETY_ON_NULL_RETURN_VAL(cache, EINA_FALSE); don't bother checking by NULL here. If it was NULL, it would not get to this point. > Source/WebKit/efl/ewk/ewk_settings.cpp:375 > + WebCore::MemoryCache* cache = WebCore::memoryCache(); > + EINA_SAFETY_ON_NULL_RETURN_VAL(cache, EINA_FALSE); same here

Comment on attachment 89181 [details] reverse logic View in context: https://bugs.webkit.org/attachment.cgi?id=89181&action=review >> Source/WebKit/efl/ewk/ewk_settings.cpp:355 >> + EINA_SAFETY_ON_NULL_RETURN_VAL(cache, EINA_FALSE); > > don't bother checking by NULL here. If it was NULL, it would not get to this point. Could explain why we don't need to check NULL here, please? Method memoryCache() is a singleton. It returns a pointer to the instance of WebCore's cache. We can't trust what a pointer this method will return.

(In reply to comment #7) > (From update of attachment 89181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89181&action=review > > >> Source/WebKit/efl/ewk/ewk_settings.cpp:355 > >> + EINA_SAFETY_ON_NULL_RETURN_VAL(cache, EINA_FALSE); > > > > don't bother checking by NULL here. If it was NULL, it would not get to this point. > > Could explain why we don't need to check NULL here, please? > Method memoryCache() is a singleton. It returns a pointer to the instance of WebCore's cache. > We can't trust what a pointer this method will return. I'm saying that the only reason this pointer could be NULL is if there was an out-of-memory condition. In that scenario, WebCore would failed to load before this function could ever be called. But I'm really nitpicking here and this is not so important.

(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 89181 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89181&action=review > > > > >> Source/WebKit/efl/ewk/ewk_settings.cpp:355 > > >> + EINA_SAFETY_ON_NULL_RETURN_VAL(cache, EINA_FALSE); > > > > > > don't bother checking by NULL here. If it was NULL, it would not get to this point. > > > > Could explain why we don't need to check NULL here, please? > > Method memoryCache() is a singleton. It returns a pointer to the instance of WebCore's cache. > > We can't trust what a pointer this method will return. > > I'm saying that the only reason this pointer could be NULL is if there was an out-of-memory condition. In that scenario, WebCore would failed to load before this function could ever be called. > Yes, I agree with you. But there is possibility to get the cache for instance in ewk function and to set the pointer on NULL, right? I know it's stupid but we can't exclude that case. > But I'm really nitpicking here and this is not so important. It's ok :)

(In reply to comment #9) > > I'm saying that the only reason this pointer could be NULL is if there was an out-of-memory condition. In that scenario, WebCore would failed to load before this function could ever be called. > > > Yes, I agree with you. But there is possibility to get the cache for instance in ewk function and to set the pointer on NULL, right? I know it's stupid but we can't exclude that case. Wooww... how would that function do that? We are returning a pointer not a pointer to pointer... Even if the stupid ewk function called delete on that pointer, subsequent calls to MemoryCache::memoryCache() would not return NULL.

(In reply to comment #10) > (In reply to comment #9) > > > I'm saying that the only reason this pointer could be NULL is if there was an out-of-memory condition. In that scenario, WebCore would failed to load before this function could ever be called. > > > > > Yes, I agree with you. But there is possibility to get the cache for instance in ewk function and to set the pointer on NULL, right? I know it's stupid but we can't exclude that case. > > Wooww... how would that function do that? We are returning a pointer not a pointer to pointer... Even if the stupid ewk function called delete on that pointer, subsequent calls to MemoryCache::memoryCache() would not return NULL. Ok, I won't say anything about pointers from now ;) I will remove checking NULL.

Created attachment 89539 [details] updated patch regarding to Lucas' suggestions Removed checking NULL in: - ewk_settings_cache_enable_get - ewk_settings_cache_enable_set - ewk_settings_cache_capacity_set Removed returned Eina_Bool value because it's useless in: - ewk_settings_cache_enable_set - ewk_settings_cache_capacity_set Added "set" to doxygen documentation

I think Antonio already review this patch. If you add Antonio Gomes to reviewer field, demarchi can decide if cq is + or -. Anyway, looks good to me.

Created attachment 89543 [details] updated changelog Adder Antonio Gomes to a reviewer list in ChangeLog

(In reply to comment #13) > I think Antonio already review this patch. If you add Antonio Gomes to reviewer field, demarchi can decide if cq is + or -. Anyway, looks good to me. However, your patch is changed considerably. You should request review again.(In reply to comment #13) > I think Antonio already review this patch. If you add Antonio Gomes to reviewer field, demarchi can decide if cq is + or -. Anyway, looks good to me. However, if your patch is changed considerably, you MUST request review again. :-)

The commit-queue encountered the following flaky tests while processing attachment 89543 [details]: http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 89543 [details] updated changelog Clearing flags on attachment: 89543 Committed r83931: <http://trac.webkit.org/changeset/83931>

All reviewed patches have been landed. Closing bug.