[webkit-reviews] review denied: [Bug 55455] [EFL] HTML saving feature : [Attachment 93287] updated patch according to Leandro's suggestions
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
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.
More information about the webkit-reviews
mailing list