Bug 106440 – [EFL][WK2] Add ewk_view_page_contents_get() API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

Add ewk_view_mhtml_data_get() API for getting mhtml data of current page.

[KwangYong Choi]

Created attachment 181892 [details]
Patch

[WebKit Review Bot]

Attachment 181892 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

Comment on attachment 181892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181892&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:941
> +    Ewk_View_MHTML_Data_Cb MHTMLDataCallback = reinterpret_cast<Ewk_View_MHTML_Data_Cb>(context);

why reinterpret_cast? static_cast should work here as far as I remember.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
> +    MHTMLDataCallback(webData.get()->size(), webData.get()->bytes());

you don't need '.get()' invoking

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:958
> +    return false;

UNUSED_PARAM ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:323
> +typedef void (*Ewk_View_MHTML_Data_Cb)(unsigned size, const unsigned char *bytes);

why not 'const char*'?

[Chris Dumez]

Comment on attachment 181892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181892&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:938
> +static void ewk_view_mhtml_data_callback(WKDataRef wkData, WKErrorRef, void* context)

I think we should use camel case here since this is coding style for non-exposed functions.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
> +    MHTMLDataCallback(webData.get()->size(), webData.get()->bytes());

webData->size(), webData->bytes() (i.e. .get() is useless)

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954
>> +                                                            ewk_view_mhtml_data_callback), false);
> 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Please fix style. Also, it would be nice to have an inline comment to indicate what 'false' stands for. e.g. false /* useBinaryEncoding */

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:323
> +typedef void (*Ewk_View_MHTML_Data_Cb)(unsigned size, const unsigned char *bytes);

Shouldn't we reverse the arguments? It feels better to me to get the array first and then its size second.

Also, why "const unsigned char *" instead of "const char *"? Technically, you are returning a string, not binary bytes. "bytes" may be a bit confusing here.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:926
> +    ASSERT_EQ(size, strlen(reinterpret_cast<const char*>(bytes)));

Question: why do we pass the size to the callback if the array is null terminated? Usually it is one or the other.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:928
> +    MHTMLDataObtained = true;

No validation of the data besides its length?

[KwangYong Choi]

Comment on attachment 181892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181892&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:938
>> +static void ewk_view_mhtml_data_callback(WKDataRef wkData, WKErrorRef, void* context)
> 
> I think we should use camel case here since this is coding style for non-exposed functions.

OK. I will use camel case here.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:941
>> +    Ewk_View_MHTML_Data_Cb MHTMLDataCallback = reinterpret_cast<Ewk_View_MHTML_Data_Cb>(context);
> 
> why reinterpret_cast? static_cast should work here as far as I remember.

static_cast is not work.

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
>>> +    MHTMLDataCallback(webData.get()->size(), webData.get()->bytes());
>> 
>> you don't need '.get()' invoking
> 
> webData->size(), webData->bytes() (i.e. .get() is useless)

OK.

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954
>>> +                                                            ewk_view_mhtml_data_callback), false);
>> 
>> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> 
> Please fix style. Also, it would be nice to have an inline comment to indicate what 'false' stands for. e.g. false /* useBinaryEncoding */

OK I will fix.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:958
>> +    return false;
> 
> UNUSED_PARAM ?

OK. I will add UNUSED_PARAM.

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:323
>>> +typedef void (*Ewk_View_MHTML_Data_Cb)(unsigned size, const unsigned char *bytes);
>> 
>> why not 'const char*'?
> 
> Shouldn't we reverse the arguments? It feels better to me to get the array first and then its size second.
> 
> Also, why "const unsigned char *" instead of "const char *"? Technically, you are returning a string, not binary bytes. "bytes" may be a bit confusing here.

OK. I will reverse the arguments. It's my work sequence. I just worked size first.
And also change const unsigned char *bytes to const char *data.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:926
>> +    ASSERT_EQ(size, strlen(reinterpret_cast<const char*>(bytes)));
> 
> Question: why do we pass the size to the callback if the array is null terminated? Usually it is one or the other.

Actually, the buffer is always null terminated string. Shall I remove the size argument?

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:928
>> +    MHTMLDataObtained = true;
> 
> No validation of the data besides its length?

OK. I will add the data comparison routine.

[KwangYong Choi]

Created attachment 182056 [details]
Patch

[KwangYong Choi]

Created attachment 182057 [details]
Patch

[Chris Dumez]

Comment on attachment 182057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182057&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:323
> +typedef void (*Ewk_View_MHTML_Data_Cb)(const char *data, unsigned length);

I would remove the length argument since data is a NULL-terminated string.

[KwangYong Choi]

Created attachment 182070 [details]
Patch

Removed length argument in callback function.

[Benjamin Poulain]

(In reply to comment #8)
> (From update of attachment 182057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182057&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:323
> > +typedef void (*Ewk_View_MHTML_Data_Cb)(const char *data, unsigned length);
> 
> I would remove the length argument since data is a NULL-terminated string.

This depends a lot on the platform, on some architecture you really don't want to read memory if you can avoid it (some ARMv7 implementations for example).

On Intel, the caches are so huge, that probably does not matter.

(I don't mind either way for this patch)

[WebKit Review Bot]

Comment on attachment 182070 [details]
Patch

Attachment 182070 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15762841

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[KwangYong Choi]

Created attachment 182086 [details]
Patch

Upload same patch again.

[Gyuyoung Kim]

Comment on attachment 182086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182086&action=review

=

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954
> +        ewkViewMHTMLDataCallback), false /* useBinaryEncoding */);

I don't know you should mention /* useBinaryEncoding */ comment. We can know what mean *false* if we see getContentsAsMHTMLData().

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:318
> + * Creates a type name for the callback function used to get the mhtml data.

s/mhtml/MHTML/g

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:320
> + * @param data string buffer of the mhtml data

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:859
> + * @param o view object to get mhtml data

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:921
> +static bool MHTMLDataObtained = false;

Generally, verb is placed first. For example, obtainedMHTMLData.

[KwangYong Choi]

Comment on attachment 182086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182086&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954
>> +        ewkViewMHTMLDataCallback), false /* useBinaryEncoding */);
> 
> I don't know you should mention /* useBinaryEncoding */ comment. We can know what mean *false* if we see getContentsAsMHTMLData().

OK. I will remove.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:318
>> + * Creates a type name for the callback function used to get the mhtml data.
> 
> s/mhtml/MHTML/g

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:320
>> + * @param data string buffer of the mhtml data
> 
> ditto.

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:859
>> + * @param o view object to get mhtml data
> 
> ditto.

OK

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:921
>> +static bool MHTMLDataObtained = false;
> 
> Generally, verb is placed first. For example, obtainedMHTMLData.

OK. I will use obtainedMHTMLData.

[KwangYong Choi]

Created attachment 182233 [details]
Patch

[KwangYong Choi]

Created attachment 182234 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 182234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182234&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:864
> +EAPI Eina_Bool ewk_view_mhtml_data_get(const Evas_Object *o, Ewk_View_MHTML_Data_Cb callback);

As you know, GTK port added *WebKitSaveMode* enum to support webpage saving functionality. I also think we need to support various data formats to save a webpage in future. I don't like to add new API to support new format. How about adding saving mode as GTK port ?

[KwangYong Choi]

Comment on attachment 182234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182234&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:864
>> +EAPI Eina_Bool ewk_view_mhtml_data_get(const Evas_Object *o, Ewk_View_MHTML_Data_Cb callback);
> 
> As you know, GTK port added *WebKitSaveMode* enum to support webpage saving functionality. I also think we need to support various data formats to save a webpage in future. I don't like to add new API to support new format. How about adding saving mode as GTK port ?

OK. I will implement mode feature.

[KwangYong Choi]

Created attachment 182271 [details]
Patch

Renamed the API name as ewk_view_page_contents_get().

And it can set the contents type for the callback data. It supports only MHTML for now.

[Gyuyoung Kim]

Comment on attachment 182271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182271&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:155
> +struct Ewk_Page_Contents_Context {

Please use _Ewk_Page_Contents_Context

[KwangYong Choi]

Created attachment 182274 [details]
Patch

struct Ewk_Page_Contents_Context -> struct _Ewk_Page_Contents_Context

[Mikhail Pozdnyakov]

(In reply to comment #20)
> (From update of attachment 182271 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182271&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:155
> > +struct Ewk_Page_Contents_Context {
> 
> Please use _Ewk_Page_Contents_Context

I guess we do not use '_' prefixes in other places. Why is it needed? (Sorry if I missed something)

[Mikhail Pozdnyakov]

Comment on attachment 182274 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182274&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:950
> +    if (!context)

it's invoked internally, how 'context' can be NULL? ASSERTION or EINA check at least is needed.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:968
> +    Ewk_Page_Contents_Context* context = static_cast<Ewk_Page_Contents_Context*>(calloc(1, sizeof(Ewk_Page_Contents_Context)));

why not 'new/delete' ?

[Gyuyoung Kim]

(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 182271 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182271&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:155
> > > +struct Ewk_Page_Contents_Context {
> > 
> > Please use _Ewk_Page_Contents_Context
> 
> I guess we do not use '_' prefixes in other places. Why is it needed? (Sorry if I missed something)

That is my mistake. I saw old version which is used by my private debugging. Sorry for my noise.

[KwangYong Choi]

Created attachment 182277 [details]
Patch

Modified to use EINA_SAFETY_ON_NULL_RETURN(context);

Modified to use new/delete.

[Mikhail Pozdnyakov]

Comment on attachment 182277 [details]
Patch

looks OK

[WebKit Review Bot]

Comment on attachment 182277 [details]
Patch

Clearing flags on attachment: 182277

Committed r139422: <http://trac.webkit.org/changeset/139422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.