Add support for getting page contents as string. Now, we can use EWK_PAGE_CONTENTS_TYPE_MHTML and EWK_PAGE_CONTENTS_TYPE_STRING for getting page contents data.
Created attachment 182502 [details] Patch
Comment on attachment 182502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182502&action=review > Source/WebKit2/ChangeLog:3 > + [EFL] Add support for getting page contents as string Missing [WK2] prefix. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:989 > + if (type == EWK_PAGE_CONTENTS_TYPE_MHTML) { Isn't switch ~ case better ?
Created attachment 183116 [details] Patch
Comment on attachment 182502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182502&action=review >> Source/WebKit2/ChangeLog:3 >> + [EFL] Add support for getting page contents as string > > Missing [WK2] prefix. OK >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:989 >> + if (type == EWK_PAGE_CONTENTS_TYPE_MHTML) { > > Isn't switch ~ case better ? OK
Comment on attachment 183116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183116&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1006 > + default: ASSERT_NOT_REACHED() ?
Comment on attachment 183116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183116&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1006 >> + default: > > ASSERT_NOT_REACHED() ? OK. I will add.
Created attachment 183119 [details] Patch
Comment on attachment 183119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183119&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:991 > + Ewk_Page_Contents_Context* context = new Ewk_Page_Contents_Context; How about delegating Ewk_Page_Contents_Context creation to application side in order to reduce duplicated code usage ? Is there any reason it should be created inside WebKit ?
Comment on attachment 183119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183119&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:991 >> + Ewk_Page_Contents_Context* context = new Ewk_Page_Contents_Context; > > How about delegating Ewk_Page_Contents_Context creation to application side in order to reduce duplicated code usage ? Is there any reason it should be created inside WebKit ? OK. I'll try.
Created attachment 183140 [details] Patch
Comment on attachment 183140 [details] Patch Attachment 183140 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15907728 New failing tests: svg/as-image/img-relative-height.html
Should I upload patch again due to false alarm?
(In reply to comment #12) > Should I upload patch again due to false alarm? I think no.
(In reply to comment #13) > (In reply to comment #12) > > Should I upload patch again due to false alarm? > > I think no. But, it would be good if you pass mac ews as well.
Created attachment 183157 [details] Patch
Comment on attachment 183157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183157&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:968 > + contentsContext->callback(contentsContext->type, webString->string().utf8().data()); Wouldn't it look more beatiful if WKEinaSharedString is used?
Comment on attachment 183157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183157&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:968 >> + contentsContext->callback(contentsContext->type, webString->string().utf8().data()); > > Wouldn't it look more beatiful if WKEinaSharedString is used? OK. I will check.
Created attachment 183307 [details] Patch
Created attachment 183366 [details] Patch Remove 'delete contentsContext;' in ewkViewPageContentsAsMHTMLCallback()
Comment on attachment 183366 [details] Patch LGTM.
Comment on attachment 183366 [details] Patch Clear r+ according to new webkit2 review policy.
Created attachment 184137 [details] Patch Added user_data to the ewk_view_page_contents_get() API. User data is commonly required for the callback functions. And I think, it's better to take parameters separately instead of taking one allocated structure.
Created attachment 184433 [details] Patch Replace internal API with C API.
Comment on attachment 184433 [details] Patch Attachment 184433 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16084579 New failing tests: fast/repaint/selection-clear.html
Comment on attachment 184433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184433&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:984 > + Ewk_Page_Contents_Context* contentsContext= static_cast<Ewk_Page_Contents_Context*>(context); Nit: space before =
Created attachment 184615 [details] Patch Style fix
Comment on attachment 184615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184615&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1004 > + Nit: WebKit hasn't added a new line between cases. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1008 > + ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:828 > + * @param user_data user data Too poor param description for user_data.
Created attachment 184925 [details] Patch Applied Gyuyoung mentioned.
Please someone r+?
Comment on attachment 184925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184925&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:944 > + ASSERT_EQ(0, strcmp(data, "Simple HTML")); Please use ASSERT_STREQ() instead, this will result in more useful output in case of failure.
Comment on attachment 184925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184925&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:944 >> + ASSERT_EQ(0, strcmp(data, "Simple HTML")); > > Please use ASSERT_STREQ() instead, this will result in more useful output in case of failure. OK.
I think bug 108634 should be landed first.
(In reply to comment #33) > I think bug 108634 should be landed first. Yes, I agree. I hope we can land that other patch soon as it keeps our bots red :/
I can not upload new patch because test_ewk2_view is failing. :( 1. ewk_view_type_check() crash 2. CoreIPC::MessageReceiverMap::dispatchMessage crash 3. ewk_view_scale_set fail
Created attachment 188792 [details] Patch Rebase and use ASSERT_STREQ().
Comment on attachment 188792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188792&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:283 > + * @param user_data user_data will be passed when ewk_view_page_contents_get is called This line use "user_data", on the other hands, 816 line uses "user data". Is it better to use same word for user_data ?
Created attachment 188800 [details] Patch
Comment on attachment 188800 [details] Patch LGTM.
Comment on attachment 188800 [details] Patch Looks fine as well.
CC'ing WK2 owners.
r+ please?
Created attachment 189684 [details] Rebased.
WebKit2 owners! Review please!
Comment on attachment 189684 [details] Rebased. Clearing flags on attachment: 189684 Committed r147700: <http://trac.webkit.org/changeset/147700>
All reviewed patches have been landed. Closing bug.