Bug 58016 – [EFL] Memory cache API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

Functions are added to public API. They allow:
1) to disable/enable memory cache,
2) to check status of memory cache.

[Grzegorz Czajkowski]

Created attachment 88586 [details]
patch

[Grzegorz Czajkowski]

Created attachment 88604 [details]
patch

ewk_setting_cache_capacity_set is added

[Gyuyoung Kim]

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()

[Grzegorz Czajkowski]

Created attachment 89181 [details]
reverse logic

[Antonio Gomes]

Comment on attachment 89181 [details]
reverse logic

Please set cq+ when it passes any API review you guys have.

[Lucas De Marchi]

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

[Grzegorz Czajkowski]

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.

[Lucas De Marchi]

(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.

[Grzegorz Czajkowski]

(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 :)

[Lucas De Marchi]

(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.

[Grzegorz Czajkowski]

(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.

[Grzegorz Czajkowski]

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

[Gyuyoung Kim]

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.

[Grzegorz Czajkowski]

Created attachment 89543 [details]
updated changelog

Adder Antonio Gomes to a reviewer list in ChangeLog

[Gyuyoung Kim]

(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. :-)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89543 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 89543 [details]
updated changelog

Clearing flags on attachment: 89543

Committed r83931: <http://trac.webkit.org/changeset/83931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.