[webkit-reviews] review denied: [Bug 55455] [EFL] HTML saving feature : [Attachment 93287] updated patch according to Leandro's suggestions

Thu May 12 09:31:59 PDT 2011

Leandro Pereira <leandro at profusion.mobi> has denied Grzegorz
<g.czajkowski at samsung.com>'s request for review:
Bug 55455: [EFL] HTML saving feature
https://bugs.webkit.org/show_bug.cgi?id=55455

Attachment 93287: updated patch according to Leandro's suggestions
https://bugs.webkit.org/attachment.cgi?id=93287&action=review

------- Additional Comments from Leandro Pereira <leandro at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=93287&action=review

> Source/WebKit/efl/ewk/ewk_frame.cpp:2024
> +Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source)

To help saving this to a file (using fwrite() and friends), returning a
positive size_t (= source_length) for success and a negative for error would be
a good idea, wouldn't it?

> Source/WebKit/efl/ewk/ewk_frame.cpp:2044
> +    WTF::String source = WTF::String("<html>") + // FIXME Html tag should be
taken from webkit.
> +	   sd->frame->document()->head()->outerHTML() +
> +	   sd->frame->document()->body()->outerHTML() +
> +	   WTF::String("</html>");

Somehow I don't like that FIXME there -- does any other port offer similar
feature? If so, how do they do it?

> Source/WebKit/efl/ewk/ewk_frame.cpp:2047
> +    *frame_source = static_cast<char*> (malloc(sizeof(char) * source_length
+ 1));

sizeof(char) is 1, by definition.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2104
> +	   char* resource_location = static_cast<char*> (malloc(sizeof(char) *
resources_location[index].length() + 1));

sizeof(char) is 1, by definition.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2111
> +	   if (!resource_location) {
> +	       CRITICAL("Could not allocate memory.");
> +	       *resources_location_list = tmp_list;
> +	       if (list_count)
> +		   *list_count = index;
> +	       return EINA_FALSE;
> +	   }

What will happen if, say, you have 10 resources, you've successfully allocated
memory for 7 of them, and the 8th one fails? The function'll return EINA_FALSE,
so the callee won't know there are 7 valid resources on resource_location_list.
Memory needs to be cleaned up here if allocation fails.

Also, I think that returning a Eina_List would be easier here.