Bug 89413

Summary: [EFL][WK2] Add ewk_view_reload_bypass_cache API.
Product: WebKit Reporter: Hyerim Bae <hyerim.bae>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cshu, kenneth, rakuco, ryuan.choi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Add ewk_view_reload_bypass_cache API
none
Rename ewk_view_reload_full.
none
Add ewk_view_reload_full API.
none
Modify the API name to ewk_view_reload_bypass_cache. none

Description Hyerim Bae 2012-06-18 19:33:17 PDT
Add ewk_view_reload_bypass_cache API.
Comment 1 Hyerim Bae 2012-06-20 00:39:54 PDT
Created attachment 148520 [details]
Add ewk_view_reload_bypass_cache API
Comment 2 Chris Dumez 2012-06-20 00:57:43 PDT
Comment on attachment 148520 [details]
Add ewk_view_reload_bypass_cache API

LGTM. Note that this function was called "ewk_view_reload_full()" in WK1. Maybe we could try to match the WK1 API as much as possible to simplify porting apps?
Comment 3 Hyerim Bae 2012-06-20 01:27:16 PDT
Yes, but this API is name like below in gtk port.

webkit_web_view_reload_bypass_cache

and I also think this is more clear.
Comment 4 Chris Dumez 2012-06-20 02:13:41 PDT
(In reply to comment #3)
> Yes, but this API is name like below in gtk port.
> 
> webkit_web_view_reload_bypass_cache
> 
> and I also think this is more clear.

Yes, I saw that gtk is using this name and I agree that it is a better name. I personally don't mind either way but I thought I would mention it since it may be a good idea to stay compatible with WK1 API unless there is a good reason not to.
Comment 5 Ryuan Choi 2012-06-20 18:32:59 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Yes, but this API is name like below in gtk port.
> > 
> > webkit_web_view_reload_bypass_cache
> > 
> > and I also think this is more clear.
> 
> Yes, I saw that gtk is using this name and I agree that it is a better name. I personally don't mind either way but I thought I would mention it since it may be a good idea to stay compatible with WK1 API unless there is a good reason not to.

I agree with chris's opinion.

As another option,
we can think about introducing webkit_web_view_reload_bypass_cache and deprecating ewk_view_load_full API.

Personally, naming itself is not important to me.
But, I believe that compatiblity looks more important.
Comment 6 Hyerim Bae 2012-06-20 18:43:48 PDT
Created attachment 148704 [details]
Rename ewk_view_reload_full.
Comment 7 Ryuan Choi 2012-06-20 18:56:11 PDT
Comment on attachment 148704 [details]
Rename ewk_view_reload_full.

View in context: https://bugs.webkit.org/attachment.cgi?id=148704&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:190
> + * @return @c EINA_TRUE on success or @c EINA_FALSE otherwise
> + */
> +EAPI Eina_Bool ewk_view_reload_full(Evas_Object *o);

It return EINA_TRUE irrespective of load.
So, this comment can be ambiguous.
Comment 8 Hyerim Bae 2012-06-20 18:59:46 PDT
(In reply to comment #7)
> (From update of attachment 148704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148704&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:190
> > + * @return @c EINA_TRUE on success or @c EINA_FALSE otherwise
> > + */
> > +EAPI Eina_Bool ewk_view_reload_full(Evas_Object *o);
> 
> It return EINA_TRUE irrespective of load.
> So, this comment can be ambiguous.

Yes, but other previous API are all the same.
Comment 9 Gyuyoung Kim 2012-06-21 17:42:06 PDT
Comment on attachment 148704 [details]
Rename ewk_view_reload_full.

View in context: https://bugs.webkit.org/attachment.cgi?id=148704&action=review

I think we don't need to use same API name with other ports. It is good to keep same API name with EFL WK1 from API compatibility of view.

> Source/WebKit2/ChangeLog:6
> +        Add API, which is for reloading documents without cache.

Generally, we place patch description below "Reviewed by NOBODY"

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:190
>>> +EAPI Eina_Bool ewk_view_reload_full(Evas_Object *o);
>> 
>> It return EINA_TRUE irrespective of load.
>> So, this comment can be ambiguous.
> 
> Yes, but other previous API are all the same.

I think we need to file a new bug for this issue. Here is not proper bug for this argument.
Comment 10 Hyerim Bae 2012-06-22 18:27:40 PDT
(In reply to comment #9)
> (From update of attachment 148704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148704&action=review
> 
> I think we don't need to use same API name with other ports. It is good to keep same API name with EFL WK1 from API compatibility of view.
> 
> > Source/WebKit2/ChangeLog:6
> > +        Add API, which is for reloading documents without cache.
> 
> Generally, we place patch description below "Reviewed by NOBODY"
> 
> >>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:190
> >>> +EAPI Eina_Bool ewk_view_reload_full(Evas_Object *o);
> >> 
> >> It return EINA_TRUE irrespective of load.
> >> So, this comment can be ambiguous.
> > 
> > Yes, but other previous API are all the same.
> 
> I think we need to file a new bug for this issue. Here is not proper bug for this argument.

Yes, I also agree with you. It seems that new bug is needed.
Comment 11 Hyerim Bae 2012-06-22 18:32:54 PDT
Created attachment 149152 [details]
Add ewk_view_reload_full API.
Comment 12 Chris Dumez 2012-06-25 05:02:44 PDT
Comment on attachment 149152 [details]
Add ewk_view_reload_full API.

LGTM.
Comment 13 Ryuan Choi 2012-07-01 22:30:42 PDT
(In reply to comment #11)
> Created an attachment (id=149152) [details]
> Add ewk_view_reload_full API.

LGTM for now.
Comment 14 Kenneth Rohde Christiansen 2012-07-04 20:00:52 PDT
Comment on attachment 149152 [details]
Add ewk_view_reload_full API.

View in context: https://bugs.webkit.org/attachment.cgi?id=149152&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:540
> +Eina_Bool ewk_view_reload_full(Evas_Object* ewkView)

I dont find the name very descriptive. What is Qt and other APIś using? _bypass_cache?
Comment 15 Hyerim Bae 2012-07-04 20:07:06 PDT
(In reply to comment #14)
> (From update of attachment 149152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149152&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:540
> > +Eina_Bool ewk_view_reload_full(Evas_Object* ewkView)
> 
> I dont find the name very descriptive. What is Qt and other APIś using? _bypass_cache?

Yes, they are using _bypass_cache.
But, Ryuan, Gyuyoung, Chris recommend to use ewk_view_reload_full for compatibility of WK1. Plz refer above their comments.
Comment 16 Kenneth Rohde Christiansen 2012-07-04 20:12:31 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 149152 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149152&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:540
> > > +Eina_Bool ewk_view_reload_full(Evas_Object* ewkView)
> > 
> > I dont find the name very descriptive. What is Qt and other APIś using? _bypass_cache?
> 
> Yes, they are using _bypass_cache.
> But, Ryuan, Gyuyoung, Chris recommend to use ewk_view_reload_full for compatibility of WK1. Plz refer above their comments.

I think you should make a future API focusing on how people use the web today and not focus on keeping legacy. The API is pretty simple anyways.
Comment 17 Ryuan Choi 2012-07-04 21:54:42 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 149152 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=149152&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:540
> > > > +Eina_Bool ewk_view_reload_full(Evas_Object* ewkView)
> > > 
> > > I dont find the name very descriptive. What is Qt and other APIś using? _bypass_cache?
> > 
> > Yes, they are using _bypass_cache.
> > But, Ryuan, Gyuyoung, Chris recommend to use ewk_view_reload_full for compatibility of WK1. Plz refer above their comments.
> 
> I think you should make a future API focusing on how people use the web today and not focus on keeping legacy. The API is pretty simple anyways.

Well, each ports use their own name for this functionality.
but they seem to keep the same name for both WK and WK2.

For example, mac port use reload and reloadFromOrigin in both WK1 and WK2.
Gtk port use webkit_web_view_reload and webkit_web_view_reload_bypass_cache in both WK1 and WK2.

WK/Efl has used ewk_view_reload_full.

By the way, I agree that this name is not good.
Does we need to change ewk_view_reload_full in WK/Efl ?
(by deprecating current and adding new one)
Comment 18 Chris Dumez 2012-07-04 23:40:55 PDT
Ryuan, since we apparently all agree that the current name is not good/expressive and since Kenneth would prefer we change the name. Why not simply rename the method for WK2?

It is not like we have a "strong" requirement to stay compatible with WK1 API and I'm sure we break compatibility in many places anyway.

The original name used by Hyerim Bae (same one as for GTK) was good IMHO (sorry Hyerim for making you change it :P).
Comment 19 Hyerim Bae 2012-07-05 00:19:35 PDT
Created attachment 150886 [details]
Modify the API name to ewk_view_reload_bypass_cache.

Modify the API name to ewk_view_reload_bypass_cache.
Comment 20 Chris Dumez 2012-07-05 00:22:07 PDT
Comment on attachment 150886 [details]
Modify the API name to ewk_view_reload_bypass_cache.

LGTM. Thanks.
Comment 21 WebKit Review Bot 2012-07-05 01:36:57 PDT
Comment on attachment 150886 [details]
Modify the API name to ewk_view_reload_bypass_cache.

Clearing flags on attachment: 150886

Committed r121892: <http://trac.webkit.org/changeset/121892>
Comment 22 WebKit Review Bot 2012-07-05 01:37:03 PDT
All reviewed patches have been landed.  Closing bug.