Bug 106752

Summary: [EFL][WK2] Add support for getting page contents as string
Product: WebKit Reporter: KwangYong Choi <ky0.choi>
Component: WebKit EFLAssignee: KwangYong Choi <ky0.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, commit-queue, dglazkov, gyuyoung.kim, jinwoo7.song, kling, lucas.de.marchi, rakuco, rniwa, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108634, 109038    
Bug Blocks: 106620, 111543    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
webkit.review.bot: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Rebased. none

Description KwangYong Choi 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.
Comment 1 KwangYong Choi 2013-01-13 21:20:07 PST
Created attachment 182502 [details]
Patch
Comment 2 Gyuyoung Kim 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 ?
Comment 3 KwangYong Choi 2013-01-16 21:19:09 PST
Created attachment 183116 [details]
Patch
Comment 4 KwangYong Choi 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
Comment 5 Gyuyoung Kim 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() ?
Comment 6 KwangYong Choi 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.
Comment 7 KwangYong Choi 2013-01-16 22:10:39 PST
Created attachment 183119 [details]
Patch
Comment 8 Gyuyoung Kim 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 ?
Comment 9 KwangYong Choi 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.
Comment 10 KwangYong Choi 2013-01-17 01:16:54 PST
Created attachment 183140 [details]
Patch
Comment 11 Build Bot 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
Comment 12 KwangYong Choi 2013-01-17 02:25:40 PST
Should I upload patch again due to false alarm?
Comment 13 Gyuyoung Kim 2013-01-17 02:26:34 PST
(In reply to comment #12)
> Should I upload patch again due to false alarm?

I think no.
Comment 14 Gyuyoung Kim 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.
Comment 15 KwangYong Choi 2013-01-17 02:38:19 PST
Created attachment 183157 [details]
Patch
Comment 16 Mikhail Pozdnyakov 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?
Comment 17 KwangYong Choi 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.
Comment 18 KwangYong Choi 2013-01-17 04:34:02 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.
Comment 19 KwangYong Choi 2013-01-17 16:20:15 PST
Created attachment 183307 [details]
Patch
Comment 20 KwangYong Choi 2013-01-17 21:13:43 PST
Created attachment 183366 [details]
Patch

Remove 'delete contentsContext;' in ewkViewPageContentsAsMHTMLCallback()
Comment 21 Gyuyoung Kim 2013-01-19 22:21:31 PST
Comment on attachment 183366 [details]
Patch

LGTM.
Comment 22 Gyuyoung Kim 2013-01-22 20:25:24 PST
Comment on attachment 183366 [details]
Patch

Clear r+ according to new webkit2 review policy.
Comment 23 KwangYong Choi 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.
Comment 24 KwangYong Choi 2013-01-24 01:03:35 PST
Created attachment 184433 [details]
Patch

Replace internal API with C API.
Comment 25 WebKit Review Bot 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
Comment 26 Ryosuke Niwa 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 =
Comment 27 KwangYong Choi 2013-01-24 17:04:04 PST
Created attachment 184615 [details]
Patch

Style fix
Comment 28 Gyuyoung Kim 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.
Comment 29 KwangYong Choi 2013-01-27 16:49:22 PST
Created attachment 184925 [details]
Patch

Applied Gyuyoung mentioned.
Comment 30 KwangYong Choi 2013-02-03 22:41:03 PST
Please someone r+?
Comment 31 Chris Dumez 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.
Comment 32 KwangYong Choi 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.
Comment 33 KwangYong Choi 2013-02-03 23:41:24 PST
I think bug 108634 should be landed first.
Comment 34 Chris Dumez 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 :/
Comment 35 KwangYong Choi 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
Comment 36 KwangYong Choi 2013-02-17 21:05:29 PST
Created attachment 188792 [details]
Patch

Rebase and use ASSERT_STREQ().
Comment 37 Gyuyoung Kim 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 ?
Comment 38 KwangYong Choi 2013-02-17 22:44:39 PST
Created attachment 188800 [details]
Patch
Comment 39 Chris Dumez 2013-02-17 22:49:49 PST
Comment on attachment 188800 [details]
Patch

LGTM.
Comment 40 Gyuyoung Kim 2013-02-17 23:08:43 PST
Comment on attachment 188800 [details]
Patch

Looks fine as well.
Comment 41 Gyuyoung Kim 2013-02-18 16:47:09 PST
CC'ing WK2 owners.
Comment 42 KwangYong Choi 2013-02-21 17:17:04 PST
r+ please?
Comment 43 KwangYong Choi 2013-02-21 22:18:04 PST
Created attachment 189684 [details]
Rebased.
Comment 44 KwangYong Choi 2013-03-29 04:49:23 PDT
WebKit2 owners! Review please!
Comment 45 WebKit Commit Bot 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>
Comment 46 WebKit Commit Bot 2013-04-04 19:50:37 PDT
All reviewed patches have been landed.  Closing bug.