Bug 90540

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