Bug 55455 – [EFL] HTML saving feature

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

Saves a html source into file given by parameter.
It also creates directory with resources.

[Grzegorz Czajkowski]

Created attachment 84208 [details]
html saving feature

[Gyuyoung Kim]

I think you need to explain why this feature is needed.

[Grzegorz Czajkowski]

(In reply to comment #0)
> Saves a html source into file given by parameter.
> It also creates directory with resources.

This feature can be useful when user wants to see a page source. It's allowed to browse web page with offline mode.

[Grzegorz Czajkowski]

(In reply to comment #3)
> (In reply to comment #0)
> > Saves a html source into file given by parameter.
> > It also creates directory with resources.
> 
> This feature can be useful when user wants to see a page source. It's allowed to browse web page with offline mode.

This feature should allow to:

1) get a source of page (without any modification of WebKit's object). Similar to desktop Browsers right click on page -> show page source.

2) save a page to disk. This function should copy needed objects for instance, images collection for modifications of paths to the local paths. API delivered in current patch modifies src attribute of images, saves page source and and the end restores default values of src. I think it's not good practise.

Let me suggest API:
1)
/**
 * Gets html source of frame.
 *
 * @param o smart frame object
 * @param frame_source stores the frame source, value should be freed after use
 * @param url URL, value should be freed after use
 */
Eina_Bool ewk_frame_source_get(Evas_Object *o, char** frame_source);

2)
/**
 * @param [in] o frame smart object
 * @param [in] path a path with the file name where a content of web page is saved, the path must exist, the file doesn't have to exist
 * @param [in][out] list_src_images a table where the src values of images are stored, if you're not intrested in that list, give @c 0, otherwise a table should be freed after use
 * @param [in][out] list_size a size of list_src_images, if you're not intrested in that size, give @c 0
 * @return @c EINA_TRUE on success, @c EINA_FALSE if the file given by param already exists or other error occured 
 */
Eina_Bool ewk_frame_dump_to_file(Evas_Object* o, const char* path, char*** list_src_images, unsigned* list_size)

Wha's your opinion on that?

[Antonio Gomes]

Comment on attachment 84208 [details]
html saving feature

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

A bunch of style to fix. please resubmit with these fixed.

> Source/WebKit/efl/ChangeLog:8
> +        Saves a html source into file given by parameter.

Lets make this change log more descriptive, like adding the TODO's, etc.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2026
> +    // save safty values to list_src_images and list_size given through param

Start the sentense with capital letter and add a "." in the end.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2079
> +        /*
> +         * This section saves images only.

Use "//" comments instead of "/* */"

> Source/WebKit/efl/ewk/ewk_frame.cpp:2080
> +         * TODO Support for others resources like scripts, plugins, external css styles.

WebKit usually uses FIXME instead of TODO.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2083
> +        // getting collection of all images in the frame

Capital in the beginning, period in the end.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2105
> +            WebCore::HTMLImageElement *imageElement = (WebCore::HTMLImageElement *) images->item(index);

* on the left side. WebKit uses static_cast for casting instead of the c-styled (XXX *).

> Source/WebKit/efl/ewk/ewk_frame.cpp:2124
> +                    // set a src attribute of the image to the local path.

Capital and period in the end.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2129
> +                // create an another name for the file with a different src attribute 

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2142
> +            // check nulls

Remove this comment. It adds nothing.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2170
> +            // set a src attribute of image to the local path

Capital letter and period in the end.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2174
> +        } // end for(...)

Remove the comment.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2176
> +        // save html source of body

Capital letter and period in the end.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2180
> +        // restore original src attributes of images

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2182
> +            WebCore::HTMLImageElement *imageElement = (WebCore::HTMLImageElement *) images->item(index);

"*" on the left side, and static_cast.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2188
> +    } else { // end if (save_resources)

Remove the comment.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2209
> +    // save a head tag

Capital letter and period.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2216
> +    // save a body tag

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2231
> +    // copy values of 'src_values' to the list given through param

Ditto

[Lucas De Marchi]

Hi Grzegorz,

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > Saves a html source into file given by parameter.
> > > It also creates directory with resources.
> > 
> > This feature can be useful when user wants to see a page source. It's allowed to browse web page with offline mode.
> 
> This feature should allow to:
> 
> 1) get a source of page (without any modification of WebKit's object). Similar to desktop Browsers right click on page -> show page source.
> 
> 2) save a page to disk. This function should copy needed objects for instance, images collection for modifications of paths to the local paths. API delivered in current patch modifies src attribute of images, saves page source and and the end restores default values of src. I think it's not good practise.
> 
> Let me suggest API:
> 1)
> /**
>  * Gets html source of frame.
>  *
>  * @param o smart frame object
>  * @param frame_source stores the frame source, value should be freed after use
>  * @param url URL, value should be freed after use
>  */
> Eina_Bool ewk_frame_source_get(Evas_Object *o, char** frame_source);
> 
> 2)
> /**
>  * @param [in] o frame smart object
>  * @param [in] path a path with the file name where a content of web page is saved, the path must exist, the file doesn't have to exist
>  * @param [in][out] list_src_images a table where the src values of images are stored, if you're not intrested in that list, give @c 0, otherwise a table should be freed after use
>  * @param [in][out] list_size a size of list_src_images, if you're not intrested in that size, give @c 0
>  * @return @c EINA_TRUE on success, @c EINA_FALSE if the file given by param already exists or other error occured 
>  */
> Eina_Bool ewk_frame_dump_to_file(Evas_Object* o, const char* path, char*** list_src_images, unsigned* list_size)
> 
> Wha's your opinion on that?

What about merging these two functions. It could be called ewk_frame_dump (or ewk_frame_source_get) and return both the source code and the list of images. It'd be the browser's role to decide what to do with them, for example saving to disk.

Also IMO it's the browser role to change the images' links, but I don't have a strong opinion about this.

[Grzegorz Czajkowski]

(In reply to comment #6)
> Hi Grzegorz,
> 
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #0)
> > > > Saves a html source into file given by parameter.
> > > > It also creates directory with resources.
> > > 
> > > This feature can be useful when user wants to see a page source. It's allowed to browse web page with offline mode.
> > 
> > This feature should allow to:
> > 
> > 1) get a source of page (without any modification of WebKit's object). Similar to desktop Browsers right click on page -> show page source.
> > 
> > 2) save a page to disk. This function should copy needed objects for instance, images collection for modifications of paths to the local paths. API delivered in current patch modifies src attribute of images, saves page source and and the end restores default values of src. I think it's not good practise.
> > 
> > Let me suggest API:
> > 1)
> > /**
> >  * Gets html source of frame.
> >  *
> >  * @param o smart frame object
> >  * @param frame_source stores the frame source, value should be freed after use
> >  * @param url URL, value should be freed after use
> >  */
> > Eina_Bool ewk_frame_source_get(Evas_Object *o, char** frame_source);
> > 
> > 2)
> > /**
> >  * @param [in] o frame smart object
> >  * @param [in] path a path with the file name where a content of web page is saved, the path must exist, the file doesn't have to exist
> >  * @param [in][out] list_src_images a table where the src values of images are stored, if you're not intrested in that list, give @c 0, otherwise a table should be freed after use
> >  * @param [in][out] list_size a size of list_src_images, if you're not intrested in that size, give @c 0
> >  * @return @c EINA_TRUE on success, @c EINA_FALSE if the file given by param already exists or other error occured 
> >  */
> > Eina_Bool ewk_frame_dump_to_file(Evas_Object* o, const char* path, char*** list_src_images, unsigned* list_size)
> > 
> > Wha's your opinion on that?
> 
> What about merging these two functions. It could be called ewk_frame_dump (or ewk_frame_source_get) and return both the source code and the list of images. It'd be the browser's role to decide what to do with them, for example saving to disk.
> 
> Also IMO it's the browser role to change the images' links, but I don't have a strong opinion about this.

Sounds reasonable.
New API:
Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source, char*** resources_list, unsigned* list_size)

 - frame_source if not NULL content of web page will be stored
 - resources_list if not NULL URL of resources will be stored (images, css styles, plugins, etc). Currently only images are supported.

If you agree with this I will sent a patch.

[Lucas De Marchi]

> > What about merging these two functions. It could be called ewk_frame_dump (or ewk_frame_source_get) and return both the source code and the list of images. It'd be the browser's role to decide what to do with them, for example saving to disk.
> > 
> > Also IMO it's the browser role to change the images' links, but I don't have a strong opinion about this.
> 
> Sounds reasonable.
> New API:
> Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source, char*** resources_list, unsigned* list_size)
> 
>  - frame_source if not NULL content of web page will be stored
>  - resources_list if not NULL URL of resources will be stored (images, css styles, plugins, etc). Currently only images are supported.
> 
> If you agree with this I will sent a patch.

Ok, this seems to be a better API. Thanks.

[Lukasz Slachciak]

Lucas, as for the new API it will look for me strange when somebody will do this

char *frame_source = 1; // must be not NULL
ewk_frame_source_get(..,&frame_source.., )

when wants to have frame_source allocated.
The same with resources_list.

Moreover, as you can see in current patch getting list of resources and getting source of page are independent things.

That's why I think we should have two separate APIs:
Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source)
Eina_Bool ewk_frame_resource_location_list_get(Evas_Object* o, char*** resources_location_list, unsigned* list_size)

[Lucas De Marchi]

(In reply to comment #9)
> Lucas, as for the new API it will look for me strange when somebody will do this
> 
> char *frame_source = 1; // must be not NULL
wrong

> ewk_frame_source_get(..,&frame_source.., )
right

Inside the function you will check for frame_source being NULL, not *frame_source. Note that it's a pointer to pointer.


> 
> when wants to have frame_source allocated.
> The same with resources_list.
> 
> Moreover, as you can see in current patch getting list of resources and getting source of page are independent things.
> 
> That's why I think we should have two separate APIs:
> Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source)
> Eina_Bool ewk_frame_resource_location_list_get(Evas_Object* o, char*** resources_location_list, unsigned* list_size)

These are ok too. What I don't want is the "_to_file" part, i.e. forcing it to be written to a file somewhere.

[Grzegorz Czajkowski]

Created attachment 92560 [details]
updated patch

API has been changed regarding to Lucas and Łukasz suggestions

[Gyuyoung Kim]

Comment on attachment 92560 [details]
updated patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:2086
> +        WTF::String srcOfImage = imageElement->src().prettyURL();

IMO,  srcOfImage needs to be changed with src_of_image. WebKit port's coding style tends to follow each port's coding style. But, some efl port's APIs doesn't adhere this rule. I think we need to decide if we change all variables of efl port with efl coding style.

[Grzegorz Czajkowski]

Created attachment 93264 [details]
Efl's coding rules are used

Could you review the patch, please?

[Leandro Pereira]

Comment on attachment 93264 [details]
Efl's coding rules are used

Just a quick, informal review.

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:2026
> +Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source)
> +{
> +    *frame_source = 0; // Saves 0 to pointer until it's not allocated.

Usually these things comes after smart_data getters and safety checks. Also, if frame_source is NULL, this will crash.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2075
> +Eina_Bool ewk_frame_resources_location_get(Evas_Object* o, char*** resources_location_list, unsigned* list_count)
> +{
> +    // Saves safty values to the output parameters.
> +    *resources_location_list = 0;
> +    if (list_count)
> +        *list_count = 0;

Same comments as above.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2101
> +    for (unsigned index = 0; index < resources_count; ++index) {
> +        tmp_list[index] = static_cast<char*> (malloc(sizeof(char) * resources_location[index].length() + 1));
> +        EINA_SAFETY_ON_NULL_RETURN_VAL(tmp_list[index], EINA_FALSE);

Possible memory leak.

[Grzegorz Czajkowski]

Created attachment 93287 [details]
updated patch according to Leandro's suggestions

[Grzegorz Czajkowski]

(In reply to comment #14)
> (From update of attachment 93264 [details])
> Just a quick, informal review.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=93264&action=review
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:2026
> > +Eina_Bool ewk_frame_source_get(Evas_Object* o, char** frame_source)
> > +{
> > +    *frame_source = 0; // Saves 0 to pointer until it's not allocated.
> 
> Usually these things comes after smart_data getters and safety checks. Also, if frame_source is NULL, this will crash.

I agree with you, fixed.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:2075
> > +Eina_Bool ewk_frame_resources_location_get(Evas_Object* o, char*** resources_location_list, unsigned* list_count)
> > +{
> > +    // Saves safty values to the output parameters.
> > +    *resources_location_list = 0;
> > +    if (list_count)
> > +        *list_count = 0;
> 
> Same comments as above.

Fixed.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:2101
> > +    for (unsigned index = 0; index < resources_count; ++index) {
> > +        tmp_list[index] = static_cast<char*> (malloc(sizeof(char) * resources_location[index].length() + 1));
> > +        EINA_SAFETY_ON_NULL_RETURN_VAL(tmp_list[index], EINA_FALSE);
> 
> Possible memory leak.

I agree with you, fixed.

[Leandro Pereira]

Comment on attachment 93287 [details]
updated patch according to Leandro's suggestions

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.

[Grzegorz Czajkowski]

Created attachment 93880 [details]
updated patch

[Grzegorz Czajkowski]

(In reply to comment #17)
> (From update of attachment 93287 [details])
> 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?

That's a good idea - fixed.

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

Ok, now html tag is taken for WebKit.

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


Fixed.

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

I agree with you, now function returns Eina_List.

[Leandro Pereira]

Comment on attachment 93880 [details]
updated patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:2040
> +    // Look for <html> tag. If it exists, the node contatins all document's source.

What will happen if it is a XHTML page? WAP page?

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

sizeof(char) = 1 :)

> Source/WebKit/efl/ewk/ewk_frame.cpp:2116
> +        if (eina_error_get()) {

eina_error_get() does not catches the possible failure in allocation due to strdup() returning NULL, only eina-related stuff, like allocating a new node for the list.

The error handling is fine, however; change just the error detection.

[Grzegorz Czajkowski]

Comment on attachment 93880 [details]
updated patch

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

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2040
>> +    // Look for <html> tag. If it exists, the node contatins all document's source.
> 
> What will happen if it is a XHTML page? WAP page?

This feature is limited to support HTML documents at the moment. You can see a checking above (line 2034). I notice about this in ChangeLog too. I have plans to extend this feature for others documents.

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

Did you mean:
malloc(source_length + sizeof(char)) ? :)

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2116
>> +        if (eina_error_get()) {
> 
> eina_error_get() does not catches the possible failure in allocation due to strdup() returning NULL, only eina-related stuff, like allocating a new node for the list.
> 
> The error handling is fine, however; change just the error detection.

Right. I will fix this.

[Leandro Pereira]

(In reply to comment #21)
> 
> Did you mean:
> malloc(source_length + sizeof(char)) ? :)
> 

No, I mean malloc(source_length + 1).

[Grzegorz Czajkowski]

Comment on attachment 93880 [details]
updated patch

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

>>> Source/WebKit/efl/ewk/ewk_frame.cpp:2040
>>> +    // Look for <html> tag. If it exists, the node contatins all document's source.
>> 
>> What will happen if it is a XHTML page? WAP page?
> 
> This feature is limited to support HTML documents at the moment. You can see a checking above (line 2034). I notice about this in ChangeLog too. I have plans to extend this feature for others documents.

This feature is limited to support HTML documents at the moment. You can see a checking above (line 2034). I notice about this in ChangeLog too. I have plans to extend this feature for others documents.

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

Did you mean:
malloc(source_length + sizeof(char)) ? :)

>>> Source/WebKit/efl/ewk/ewk_frame.cpp:2116
>>> +        if (eina_error_get()) {
>> 
>> eina_error_get() does not catches the possible failure in allocation due to strdup() returning NULL, only eina-related stuff, like allocating a new node for the list.
>> 
>> The error handling is fine, however; change just the error detection.
> 
> Right. I will fix this.

What do you say of using goto? We have to free the all Eina_List if we can not allocate memory (while strdup and allocating a new node for the list. My suggestion is:

    char* duplicate = strdup(img_location.utf8().data());
    if (!duplicate)
        goto error;
    img_location_list = eina_list_append(img_location_list, duplicate);
    if (eina_error_get()) 
        goto error;


// Out of memory handling.
error:
    CRITICAL("Could not allocate memory.");
    EINA_LIST_FREE(img_location_list, data)
        free(data);
    return 0;

[Leandro Pereira]

(In reply to comment #23)
> 
> What do you say of using goto? We have to free the all Eina_List if we can not allocate memory (while strdup and allocating a new node for the list. My suggestion is:
> 

Goto for error handling is a pretty common pattern inside EFL, and since this is on a port-related file, it is fine. I'd use another label name, however, like 'out_of_memory_handler' or something.

[Grzegorz Czajkowski]

Created attachment 94577 [details]
updated patch

Added out of memory handling.

[Leandro Pereira]

(In reply to comment #25)
> 
> Added out of memory handling.
>

LGTM.

[Antonio Gomes]

Comment on attachment 94577 [details]
updated patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:2014
> + * It's part of html saving feature. Currently only HTML documents are supported.

html x HTML

> Source/WebKit/efl/ewk/ewk_frame.cpp:2036
> +        WRN("Only html documents are supported");

HTML

> Source/WebKit/efl/ewk/ewk_frame.cpp:2041
> +    WebCore::Node* de = sd->frame->document()->documentElement();

de -> documentElement

> Source/WebKit/efl/ewk/ewk_frame.cpp:2043
> +        for (WebCore::Node* e = de->firstChild(); e; e = e->parentElement()) {

e -> node or currentNode or whatever more descriptive.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2099
> +        WebCore::HTMLImageElement* img_element = static_cast<WebCore::HTMLImageElement*>(images->item(index));

camel case -> imageElement?

> Source/WebKit/efl/ewk/ewk_frame.cpp:2103
> +        WTF::String img_location = img_element->src().prettyURL();

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2105
> +        Eina_List* l = 0;

l -> listOfXXX

> Source/WebKit/efl/ewk/ewk_frame.cpp:2115
> +        if (!duplicate)

duplicate -> imageLocationCopy?

[Grzegorz Czajkowski]

Comment on attachment 94577 [details]
updated patch

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

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2014
>> + * It's part of html saving feature. Currently only HTML documents are supported.
> 
> html x HTML

Fixed.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2036
>> +        WRN("Only html documents are supported");
> 
> HTML

Fixed.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2041
>> +    WebCore::Node* de = sd->frame->document()->documentElement();
> 
> de -> documentElement

Fixed.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2043
>> +        for (WebCore::Node* e = de->firstChild(); e; e = e->parentElement()) {
> 
> e -> node or currentNode or whatever more descriptive.

Fixed (node).

> Source/WebKit/efl/ewk/ewk_frame.cpp:2077
> + * It's part of html saving feature. Currently only locations of images are supported.

Fixed html -> HTML.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2094
> +    Eina_List* img_location_list = 0;

Changeg to listOfImagesLocation.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2099
>> +        WebCore::HTMLImageElement* img_element = static_cast<WebCore::HTMLImageElement*>(images->item(index));
> 
> camel case -> imageElement?

Fixed (imageElement).

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2103
>> +        WTF::String img_location = img_element->src().prettyURL();
> 
> Ditto.

Fixed imageLocation.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2105
>> +        Eina_List* l = 0;
> 
> l -> listOfXXX

This is only temp iterator of list.
Changeg to listIterator.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:2115
>> +        if (!duplicate)
> 
> duplicate -> imageLocationCopy?

Fixed (imageLocationCopy).

[Grzegorz Czajkowski]

Created attachment 97413 [details]
updated patch

[Gyuyoung Kim]

Comment on attachment 97413 [details]
updated patch

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

LGTM except for my comments.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1983
> + *      must @b not be @c 0, this value @b should be freed after use

Move start of must to start of frame_source below.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2000
> +        // FIXME Support others documents.

Missing ":" in FIXME.

> Source/WebKit/efl/ewk/ewk_frame.cpp:2048
> + *   the Eina_List should be freed after use

wrong indentation. Move the Eina_List to @c below.

[Grzegorz Czajkowski]

Created attachment 103233 [details]
updated patch

Updated patch according to Gyuyoung's comments.
Additionally, Patch has been:
 - adjusted to the EFL's coding style and documentation of HTML saving feature was moved to the header file.

[Leandro Pereira]

(In reply to comment #31)
> Created an attachment (id=103233) [details]
> updated patch
> 
> Updated patch according to Gyuyoung's comments.
>

Informal r+.

[Antonio Gomes]

Comment on attachment 103233 [details]
updated patch

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

cpp files should go into WebKit coding style and public headers with toolkit coding style, no?

Based on that some comments...

> Source/WebKit/efl/ewk/ewk_frame.cpp:1495
> +    *frame_source = 0; // Saves 0 to pointer until it's not allocated.

frameSource?

> Source/WebKit/efl/ewk/ewk_frame.cpp:1504
> +    WebCore::Node *documentNode = sd->frame->document()->documentElement();

* position

> Source/WebKit/efl/ewk/ewk_frame.cpp:1508
> +                WebCore::HTMLElement *element = static_cast<WebCore::HTMLElement*>(node);

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1532
> +    (*frame_source)[source_length] = '\0';

who deletes "frame_source"?

> Source/WebKit/efl/ewk/ewk_frame.cpp:1537
> +Eina_List *ewk_frame_resources_location_get(Evas_Object *o)

* position

> Source/WebKit/efl/ewk/ewk_frame.cpp:1543
> +    Eina_List *listOfImagesLocation = 0;

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1554
> +        Eina_List *listIterator = 0;

Ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:1555
> +        void *data = 0;

Ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:1563
> +        char *imageLocationCopy = strdup(imageLocation.utf8().data());

Ditto

[Grzegorz Czajkowski]

(In reply to comment #33)
> (From update of attachment 103233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103233&action=review
> 
> cpp files should go into WebKit coding style and public headers with toolkit coding style, no?

Thanks for your review.
There were commits which move the pointer operator to variable according to EFL coding style for all WebKit-EFL's files. For instance, commit for ewk_frame https://bugs.webkit.org/show_bug.cgi?id=65357

So I adjusted this patch to these changes.

> 
> Based on that some comments...
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1495
> > +    *frame_source = 0; // Saves 0 to pointer until it's not allocated.
> 
> frameSource?

This is the pointer where to store source of frame which is given through parameter:
EAPI ssize_t ewk_frame_source_get(Evas_Object *o, char **frame_source);
Generally API exposed by WebKit-EFL uses names separated by underline.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1504
> > +    WebCore::Node *documentNode = sd->frame->document()->documentElement();
> 
> * position

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1508
> > +                WebCore::HTMLElement *element = static_cast<WebCore::HTMLElement*>(node);
> 
> Ditto.

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1532
> > +    (*frame_source)[source_length] = '\0';
> 
> who deletes "frame_source"?

Caller should delete "frame_Source". It's mentioned in doxygen description.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1537
> > +Eina_List *ewk_frame_resources_location_get(Evas_Object *o)
> 
> * position

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1543
> > +    Eina_List *listOfImagesLocation = 0;
> 
> Ditto.

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1554
> > +        Eina_List *listIterator = 0;
> 
> Ditto

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1555
> > +        void *data = 0;
> 
> Ditto

EFL's coding style.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1563
> > +        char *imageLocationCopy = strdup(imageLocation.utf8().data());
> 
> Ditto

EFL's coding style.

[Gyuyoung Kim]

Comment on attachment 103233 [details]
updated patch

This patch have reviewed so far intensively. I think Antonio's comment is enough to be answered by Grzegorz. I think there are no critical problems in this patch. So, I land this patch.

[WebKit Review Bot]

Comment on attachment 103233 [details]
updated patch

Clearing flags on attachment: 103233

Committed r92948: <http://trac.webkit.org/changeset/92948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.