[webkit-reviews] review denied: [Bug 106620] [EFL] Add support for MHTML save/load feature to MiniBrowser : [Attachment 205175] Patch

[bugzilla-daemon at webkit.org]

Christophe Dumez <dchris at gmail.com> has denied Santosh Mahto
<santosh.ma at samsung.com>'s request for review:
Bug 106620: [EFL] Add support for MHTML save/load feature to MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=106620

Attachment 205175: Patch
https://bugs.webkit.org/attachment.cgi?id=205175&action=review

------- Additional Comments from Christophe Dumez <dchris at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205175&action=review


> Tools/MiniBrowser/efl/main.c:361
> +    Eina_Strbuf *fileNameWithMht = eina_strbuf_new();

This is created even if we don't need it :(

> Tools/MiniBrowser/efl/main.c:362
> +    if (strlen(fileName) <= 4 || strcmp(fileName + strlen(fileName) - 4,
".mht")) {

eina_str_has_suffix() is your friend:
http://docs.enlightenment.org/auto/eina/group__Eina__String__Group.html#ga1e84d
e127ac0214f4983daeea3c3abf8

> Tools/MiniBrowser/efl/main.c:378
> +    info("SUCCESS: saved.....\n");

I think one period would suffice.

> Tools/MiniBrowser/efl/main.c:462
> +	   info("Save page (Ctrl + s) was pressed. Current page will be saved
to %s\n", save_file_name);

I think you should append .mht before this message and not in the callback or
this message will be wrong. Alternatively, you can move the last part of the
message to the callback.

> Tools/MiniBrowser/efl/main.c:1021
> +    elm_fileselector_entry_is_save_set(fs_entry, EINA_TRUE);

huh? What if type == LOAD?

> Tools/MiniBrowser/efl/main.c:1025
> +	   eina_strbuf_append_printf(default_file, "%s/%s.mht", getenv("HOME"),
ewk_view_title_get(window->ewk_view));

This is a generic function. There should be no mention of mht here. Add an
argument to show_file_entry_dialog() if necessary.