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

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

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.

[KwangYong Choi]

Created attachment 182502 [details]
Patch

[Gyuyoung Kim]

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]

Created attachment 183116 [details]
Patch

[KwangYong Choi]

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 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 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]

Created attachment 183119 [details]
Patch

[Gyuyoung Kim]

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 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]

Created attachment 183140 [details]
Patch

[Build Bot]

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]

Should I upload patch again due to false alarm?

[Gyuyoung Kim]

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

I think no.

[Gyuyoung Kim]

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

Created attachment 183157 [details]
Patch

[Mikhail Pozdnyakov]

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 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 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]

Created attachment 183307 [details]
Patch

[KwangYong Choi]

Created attachment 183366 [details]
Patch

Remove 'delete contentsContext;' in ewkViewPageContentsAsMHTMLCallback()

[Gyuyoung Kim]

Comment on attachment 183366 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 183366 [details]
Patch

Clear r+ according to new webkit2 review policy.

[KwangYong Choi]

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]

Created attachment 184433 [details]
Patch

Replace internal API with C API.

[WebKit Review Bot]

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 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]

Created attachment 184615 [details]
Patch

Style fix

[Gyuyoung Kim]

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]

Created attachment 184925 [details]
Patch

Applied Gyuyoung mentioned.

[KwangYong Choi]

Please someone r+?

[Chris Dumez]

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 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]

I think bug 108634 should be landed first.

[Chris Dumez]

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

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]

Created attachment 188792 [details]
Patch

Rebase and use ASSERT_STREQ().

[Gyuyoung Kim]

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]

Created attachment 188800 [details]
Patch

[Chris Dumez]

Comment on attachment 188800 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 188800 [details]
Patch

Looks fine as well.

[Gyuyoung Kim]

CC'ing WK2 owners.

[KwangYong Choi]

r+ please?

[KwangYong Choi]

Created attachment 189684 [details]
Rebased.

[KwangYong Choi]

WebKit2 owners! Review please!

[WebKit Commit Bot]

Comment on attachment 189684 [details]
Rebased.

Clearing flags on attachment: 189684

Committed r147700: <http://trac.webkit.org/changeset/147700>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.