106752 – [EFL][WK2] Add support for getting page contents as string

RESOLVED FIXED 106752

[EFL][WK2] Add support for getting page contents as string

https://bugs.webkit.org/show_bug.cgi?id=106752

Summary [EFL][WK2] Add support for getting page contents as string

KwangYong Choi

Reported 2013-01-13 21:05:22 PST

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.

Attachments
Patch (6.01 KB, patch) 2013-01-13 21:20 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (6.28 KB, patch) 2013-01-16 21:19 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (6.31 KB, patch) 2013-01-16 22:10 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (8.89 KB, patch) 2013-01-17 01:16 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (8.89 KB, patch) 2013-01-17 02:38 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (8.83 KB, patch) 2013-01-17 16:20 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (8.70 KB, patch) 2013-01-17 21:13 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (7.81 KB, patch) 2013-01-22 21:44 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (7.81 KB, patch) 2013-01-24 01:03 PST, KwangYong Choi	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Patch (7.91 KB, patch) 2013-01-24 17:04 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (7.95 KB, patch) 2013-01-27 16:49 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (7.82 KB, patch) 2013-02-17 21:05 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Patch (7.82 KB, patch) 2013-02-17 22:44 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Rebased. (7.92 KB, patch) 2013-02-21 22:18 PST, KwangYong Choi	no flags	Details Formatted Diff Diff
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

KwangYong Choi

Comment 1 2013-01-13 21:20:07 PST

Created attachment 182502 [details] Patch

Gyuyoung Kim

Comment 2 2013-01-16 18:30:12 PST

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 ?

KwangYong Choi

Comment 3 2013-01-16 21:19:09 PST

Created attachment 183116 [details] Patch

KwangYong Choi

Comment 4 2013-01-16 21:19:49 PST

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

Gyuyoung Kim

Comment 5 2013-01-16 22:06:16 PST

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

KwangYong Choi

Comment 6 2013-01-16 22:08:16 PST

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.

KwangYong Choi

Comment 7 2013-01-16 22:10:39 PST

Created attachment 183119 [details] Patch

Gyuyoung Kim

Comment 8 2013-01-16 22:52:44 PST

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 ?

KwangYong Choi

Comment 9 2013-01-17 00:30:08 PST

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.

KwangYong Choi

Comment 10 2013-01-17 01:16:54 PST

Created attachment 183140 [details] Patch

Build Bot

Comment 11 2013-01-17 01:43:49 PST

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

KwangYong Choi

Comment 12 2013-01-17 02:25:40 PST

Should I upload patch again due to false alarm?

Gyuyoung Kim

Comment 13 2013-01-17 02:26:34 PST

(In reply to comment #12) > Should I upload patch again due to false alarm? I think no.

Gyuyoung Kim

Comment 14 2013-01-17 02:29:04 PST

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

KwangYong Choi

Comment 15 2013-01-17 02:38:19 PST

Created attachment 183157 [details] Patch

Mikhail Pozdnyakov

Comment 16 2013-01-17 04:01:24 PST

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?

KwangYong Choi

Comment 17 2013-01-17 04:34:00 PST

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.

KwangYong Choi

Comment 18 2013-01-17 04:34:02 PST

KwangYong Choi

Comment 19 2013-01-17 16:20:15 PST

Created attachment 183307 [details] Patch

KwangYong Choi

Comment 20 2013-01-17 21:13:43 PST

Created attachment 183366 [details] Patch Remove 'delete contentsContext;' in ewkViewPageContentsAsMHTMLCallback()

Gyuyoung Kim

Comment 21 2013-01-19 22:21:31 PST

Comment on attachment 183366 [details] Patch LGTM.

Gyuyoung Kim

Comment 22 2013-01-22 20:25:24 PST

Comment on attachment 183366 [details] Patch Clear r+ according to new webkit2 review policy.

KwangYong Choi

Comment 23 2013-01-22 21:44:21 PST

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.

KwangYong Choi

Comment 24 2013-01-24 01:03:35 PST

Created attachment 184433 [details] Patch Replace internal API with C API.

WebKit Review Bot

Comment 25 2013-01-24 02:46:25 PST

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

Ryosuke Niwa

Comment 26 2013-01-24 10:47:58 PST

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 =

KwangYong Choi

Comment 27 2013-01-24 17:04:04 PST

Created attachment 184615 [details] Patch Style fix

Gyuyoung Kim

Comment 28 2013-01-25 22:11:25 PST

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.

KwangYong Choi

Comment 29 2013-01-27 16:49:22 PST

Created attachment 184925 [details] Patch Applied Gyuyoung mentioned.

KwangYong Choi

Comment 30 2013-02-03 22:41:03 PST

Please someone r+?

Chris Dumez

Comment 31 2013-02-03 22:46:31 PST

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.

KwangYong Choi

Comment 32 2013-02-03 22:47:57 PST

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.

KwangYong Choi

Comment 33 2013-02-03 23:41:24 PST

I think bug 108634 should be landed first.

Chris Dumez

Comment 34 2013-02-03 23:50:57 PST

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

KwangYong Choi

Comment 35 2013-02-07 19:59:23 PST

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

KwangYong Choi

Comment 36 2013-02-17 21:05:29 PST

Created attachment 188792 [details] Patch Rebase and use ASSERT_STREQ().

Gyuyoung Kim

Comment 37 2013-02-17 21:54:52 PST

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 ?

KwangYong Choi

Comment 38 2013-02-17 22:44:39 PST

Created attachment 188800 [details] Patch

Chris Dumez

Comment 39 2013-02-17 22:49:49 PST

Comment on attachment 188800 [details] Patch LGTM.

Gyuyoung Kim

Comment 40 2013-02-17 23:08:43 PST

Comment on attachment 188800 [details] Patch Looks fine as well.

Gyuyoung Kim

Comment 41 2013-02-18 16:47:09 PST

CC'ing WK2 owners.

KwangYong Choi

Comment 42 2013-02-21 17:17:04 PST

r+ please?

KwangYong Choi

Comment 43 2013-02-21 22:18:04 PST

Created attachment 189684 [details] Rebased.

KwangYong Choi

Comment 44 2013-03-29 04:49:23 PDT

WebKit2 owners! Review please!

WebKit Commit Bot

Comment 45 2013-04-04 19:50:31 PDT

Comment on attachment 189684 [details] Rebased. Clearing flags on attachment: 189684 Committed r147700: <http://trac.webkit.org/changeset/147700>

WebKit Commit Bot

Comment 46 2013-04-04 19:50:37 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebKit EFL

Assignee

KwangYong Choi

Reported

2013-01-13 21:05 PST

Modified

2013-04-04 19:50 PDT History

CC List

13 users Show

URL

Keywords

Depends on

108634 109038

Blocks

106620 111543

Dependencies

tree graph