Summary: | [WK2][EFL] Ewk_View needs API to load HTML data | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | hyerim.bae, kenneth, ryuan.choi, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 61838 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2012-07-04 03:53:08 PDT
Created attachment 150764 [details]
Patch
Comment on attachment 150764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150764&action=review > Source/WebKit2/ChangeLog:4 > + [WK2][EFL] MiniBrowser needs to handle load errors > + https://bugs.webkit.org/show_bug.cgi?id=90540 IMO, more important change of this patch looks adding new API, right? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 > +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) In WK1/Efl, we used ewk_frame_contents_set and ewk_frame_contents_alternate_set for same feature. http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_frame.cpp#L422 Comment on attachment 150764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150764&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 >> +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) > > In WK1/Efl, we used ewk_frame_contents_set and ewk_frame_contents_alternate_set for same feature. > > http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_frame.cpp#L422 In WK2, we don't have frame-level APIs anymore so we are breaking compatibility with WK1 API anyway. Since we break compatibility, I decided to simplify the API a bit and propose something similar to what is in Qt port. The method I'm proposing can achieve the same behavior as the 2 methods you mention. We don't loose any functionality. (In reply to comment #3) > (From update of attachment 150764 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150764&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 > >> +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) > > > > In WK1/Efl, we used ewk_frame_contents_set and ewk_frame_contents_alternate_set for same feature. > > > > http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_frame.cpp#L422 > > In WK2, we don't have frame-level APIs anymore so we are breaking compatibility with WK1 API anyway. Since we break compatibility, I decided to simplify the API a bit and propose something similar to what is in Qt port. > The method I'm proposing can achieve the same behavior as the 2 methods you mention. We don't loose any functionality. Although we don't break compatibility, is it better to use similar name for application developers to easily understand ? I think that 1 method looks enough. (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 150764 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=150764&action=review > > > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 > > >> +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) > > > > > > In WK1/Efl, we used ewk_frame_contents_set and ewk_frame_contents_alternate_set for same feature. > > > > > > http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_frame.cpp#L422 > > > > In WK2, we don't have frame-level APIs anymore so we are breaking compatibility with WK1 API anyway. Since we break compatibility, I decided to simplify the API a bit and propose something similar to what is in Qt port. > > The method I'm proposing can achieve the same behavior as the 2 methods you mention. We don't loose any functionality. > > Although we don't break compatibility, is it better to use similar name for application developers to easily understand ? > > I think that 1 method looks enough. Yes, I usually try to keep compatibility or at least stay close to the WK1 API. But in this case, I think it is a good opportunity to simplify the API. Also, I don't really like the WK1 function names for this and I don't find them clear enough. Unless there is strong opinion on Samsung side against it, I would like to land this patch as it is. Created attachment 150783 [details]
Patch
Update bug title in Changelogs as Ryuan suggested.
Comment on attachment 150783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150783&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 > +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) html_data_load ? btw, wouldnt it load svg as well? Comment on attachment 150783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150783&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 >> +Eina_Bool ewk_view_html_load(Evas_Object* ewkView, const char* html, const char* baseUrl, const char* unreachableUrl) > > html_data_load ? btw, wouldnt it load svg as well? html_data_load(), why not if you prefer. I would even go for html_string_load() then. if you look at the methods I'm calling internally: loadHTMLString(), this would map quite closely. If it does load SVG, then the page API is confusing :) I am all for string_load :-) Created attachment 150881 [details]
Patch
Take Kenneth's feedback into consideration.
Comment on attachment 150881 [details] Patch Clearing flags on attachment: 150881 Committed r121890: <http://trac.webkit.org/changeset/121890> All reviewed patches have been landed. Closing bug. *** Bug 91352 has been marked as a duplicate of this bug. *** |