Bug 106440

Created attachment 181892 [details] Patch

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.

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*'?

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?

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.

Created attachment 182056 [details] Patch

Created attachment 182057 [details] Patch

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.

Created attachment 182070 [details] Patch Removed length argument in callback function.

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

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

Created attachment 182086 [details] Patch Upload same patch again.

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.

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.

Created attachment 182233 [details] Patch

Created attachment 182234 [details] Patch

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 ?

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.

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.

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

Created attachment 182274 [details] Patch struct Ewk_Page_Contents_Context -> struct _Ewk_Page_Contents_Context

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

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' ?

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

Created attachment 182277 [details] Patch Modified to use EINA_SAFETY_ON_NULL_RETURN(context); Modified to use new/delete.

Comment on attachment 182277 [details] Patch looks OK

Comment on attachment 182277 [details] Patch Clearing flags on attachment: 182277 Committed r139422: <http://trac.webkit.org/changeset/139422>

All reviewed patches have been landed. Closing bug.