Bug 106620 – [EFL] Add support for MHTML save/load feature to MiniBrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

Add support for MHTML save/load feature to MiniBrowser.

[KwangYong Choi]

Created attachment 182501 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 182501 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:31
> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";

Can't we show an input popup to get page name ?

[KwangYong Choi]

Comment on attachment 182501 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:31
>> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";
> 
> Can't we show an input popup to get page name ?

I know, for elementary, it's not simple. I should make popup, layout and the contents of itself for the simple popup.

How about keep this simple implementation for the MiniBrowser? This is the minimal save and load feature of the implementation.

[Gyuyoung Kim]

Comment on attachment 182501 [details]
Patch

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

>>> Tools/MiniBrowser/efl/main.c:31
>>> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";
>> 
>> Can't we show an input popup to get page name ?
> 
> I know, for elementary, it's not simple. I should make popup, layout and the contents of itself for the simple popup.
> 
> How about keep this simple implementation for the MiniBrowser? This is the minimal save and load feature of the implementation.

If there is no objection from others, I'm ok. But, please support it in future.

> Tools/MiniBrowser/efl/main.c:272
> +    eet_write(ef, "MHTML data", data, strlen(data), 0 /* compress */);

We have used NULL instead of 0 in MiniBrowser.

[KwangYong Choi]

Comment on attachment 182501 [details]
Patch

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

>>>> Tools/MiniBrowser/efl/main.c:31
>>>> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";
>>> 
>>> Can't we show an input popup to get page name ?
>> 
>> I know, for elementary, it's not simple. I should make popup, layout and the contents of itself for the simple popup.
>> 
>> How about keep this simple implementation for the MiniBrowser? This is the minimal save and load feature of the implementation.
> 
> If there is no objection from others, I'm ok. But, please support it in future.

OK.

>> Tools/MiniBrowser/efl/main.c:272
>> +    eet_write(ef, "MHTML data", data, strlen(data), 0 /* compress */);
> 
> We have used NULL instead of 0 in MiniBrowser.

The type of the last parameter 'compress' is int. So, I think, 0 is correct here.

int eet_write	(Eet_File * ef,
const char * 	name,
const void * 	data,
int 	size,
int 	compress 
)

[Gyuyoung Kim]

(In reply to comment #5)
 
> >> Tools/MiniBrowser/efl/main.c:272
> >> +    eet_write(ef, "MHTML data", data, strlen(data), 0 /* compress */);
> > 
> > We have used NULL instead of 0 in MiniBrowser.
> 
> The type of the last parameter 'compress' is int. So, I think, 0 is correct here.

Aha, ok.

[Gyuyoung Kim]

Comment on attachment 182501 [details]
Patch

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

>>>>> Tools/MiniBrowser/efl/main.c:31
>>>>> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";
>>>> 
>>>> Can't we show an input popup to get page name ?
>>> 
>>> I know, for elementary, it's not simple. I should make popup, layout and the contents of itself for the simple popup.
>>> 
>>> How about keep this simple implementation for the MiniBrowser? This is the minimal save and load feature of the implementation.
>> 
>> If there is no objection from others, I'm ok. But, please support it in future.
> 
> OK.

It looks it is not difficult to support an input popup dialog. on_javascript_alert() already implemented a popup dialog based on elementary. And, it seems to me that you can add input field using elm_entry_add(popup) and elm_object_context_set(popup, entry).

[KwangYong Choi]

Comment on attachment 182501 [details]
Patch

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

>>>>>> Tools/MiniBrowser/efl/main.c:31
>>>>>> +static const char SAVED_CONTENTS_PATH[] = "/tmp/page.mht";
>>>>> 
>>>>> Can't we show an input popup to get page name ?
>>>> 
>>>> I know, for elementary, it's not simple. I should make popup, layout and the contents of itself for the simple popup.
>>>> 
>>>> How about keep this simple implementation for the MiniBrowser? This is the minimal save and load feature of the implementation.
>>> 
>>> If there is no objection from others, I'm ok. But, please support it in future.
>> 
>> OK.
> 
> It looks it is not difficult to support an input popup dialog. on_javascript_alert() already implemented a popup dialog based on elementary. And, it seems to me that you can add input field using elm_entry_add(popup) and elm_object_context_set(popup, entry).

OK. I will add.

>>> Tools/MiniBrowser/efl/main.c:272
>>> +    eet_write(ef, "MHTML data", data, strlen(data), 0 /* compress */);
>> 
>> We have used NULL instead of 0 in MiniBrowser.
> 
> The type of the last parameter 'compress' is int. So, I think, 0 is correct here.
> 
> int eet_write    (Eet_File * ef,
> const char *     name,
> const void *     data,
> int     size,
> int     compress 
> )

eet_write() writes 'name' also into the file. I may use NULL instead of "MHTML data".

[Santosh Mahto]

Created attachment 204782 [details]
Patch

[Santosh Mahto]

I have upload the new patch
CTRL +  S  will launch file entry popup to enter the filename/path and save the  page with chosen filename.
CTRL + L   Will  also file entry popup to select file to load in current view

[Gyuyoung Kim]

Comment on attachment 204782 [details]
Patch

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

> Tools/ChangeLog:9
> +        from filesystem

Add a new line.

> Tools/MiniBrowser/efl/main.c:28
> +#include <Eet.h>

Wrong alphabet order.

> Tools/MiniBrowser/efl/main.c:353
> +static void page_contents_callback(Ewk_Page_Contents_Type type, const char *data, void* user_data)

Wrong * place in void*

> Tools/MiniBrowser/efl/main.c:365
> +static char* show_file_entry_dialog(Browser_Window *window);

It would be good if you declare function with other function declarations. 173 line ?

[Chris Dumez]

Comment on attachment 204782 [details]
Patch

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

>> Tools/ChangeLog:9
>> +        from filesystem
> 
> Add a new line.

and period.

> Tools/MiniBrowser/efl/main.c:441
> +        char *save_file_name = show_file_entry_dialog(window);

never freed?

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

never freed?

> Tools/MiniBrowser/efl/main.c:452
> +        info("Load page (Ctrl + l) was pressed. Loading  %s \n", eina_strbuf_string_get(uri_path));

extra space before \n and before %s

> Tools/MiniBrowser/efl/main.c:1021
> +    const char *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;

already a stringshare so eina_stringshare_ref() would suffice

[Santosh Mahto]

Created attachment 204908 [details]
Patch

[Santosh Mahto]

> 
> > Tools/MiniBrowser/efl/main.c:441
> > +        char *save_file_name = show_file_entry_dialog(window);
> 
> never freed
I tried to free it in callback but got efl crash. i also tried in few more places 
but faced crash.
I doubt no free is required. could you give your suggestion 

> 
> > Tools/MiniBrowser/efl/main.c:450
> > +        Eina_Strbuf *uri_path = eina_strbuf_new();
> 
> never freed?
 Done

[Chris Dumez]

(In reply to comment #14)
> > 
> > > Tools/MiniBrowser/efl/main.c:441
> > > +        char *save_file_name = show_file_entry_dialog(window);
> > 
> > never freed
> I tried to free it in callback but got efl crash. i also tried in few more places 
> but faced crash.
> I doubt no free is required. could you give your suggestion 

As I said, it is already a stringshare so you need eina_stringshare_del() not free().

[Chris Dumez]

Comment on attachment 204908 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:441
> +        char *save_file_name = show_file_entry_dialog(window);

you need to eina_stringshare_del() after use.

> Tools/MiniBrowser/efl/main.c:979
> +static char*

should return a Eina_Stringshare *

> Tools/MiniBrowser/efl/main.c:1022
> +    const char *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;

should use eina_stringshare_ref() not eina_stringshare_add() as this is already a stringshare according to the doc.

[Santosh Mahto]

> 
> > Tools/MiniBrowser/efl/main.c:1022
> > +    const char *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;
> 
> should use eina_stringshare_ref() not eina_stringshare_add() as this is already a stringshare according to the doc.

No its not 
return type of elm_fileselector_entry_path_get(fs_entry) is const char* so we need to use eina_stringshare_add to use it as shared sting

[Santosh Mahto]

Created attachment 204988 [details]
Patch

[Chris Dumez]

(In reply to comment #17)
> > 
> > > Tools/MiniBrowser/efl/main.c:1022
> > > +    const char *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;
> > 
> > should use eina_stringshare_ref() not eina_stringshare_add() as this is already a stringshare according to the doc.
> 
> No its not 
> return type of elm_fileselector_entry_path_get(fs_entry) is const char* so we need to use eina_stringshare_add to use it as shared sting

Damn, I was confusing it with elm_fileselector_path_get(), sorry:
http://docs.enlightenment.org/stable/elementary/group__Fileselector.html#ga070960b00a8317519dc00076d07574ea

For the record, returning a const char* does not indicate that it is not a stringshare. You need to look at the doc for that since a stringshare* is also a const char*.

Ok, so I agree that eina_stringshare_add() is needed. However, eina_stringshare_del() is needed after use.

[Chris Dumez]

Comment on attachment 204988 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:358
> +    ef = eet_open((Eina_Stringshare *)user_data, EET_FILE_MODE_WRITE);

maybe my own personal taste but I think it would look more readable if we stored the results of this cast in a local variable.

> Tools/MiniBrowser/efl/main.c:362
> +    eina_stringshare_del((Eina_Stringshare *)user_data);

ditto.

> Tools/MiniBrowser/efl/main.c:442
> +        char *save_file_name = show_file_entry_dialog(window);

Eina_Stringshare *save_file_name =

> Tools/MiniBrowser/efl/main.c:448
> +        Eina_Stringshare *open_file_name = show_file_entry_dialog(window);

need to eina_stringshare_del() after use.

> Tools/MiniBrowser/efl/main.c:1023
> +    const char *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;

Eina_Stringshare *file_path

[Santosh Mahto]

Created attachment 204996 [details]
Patch

[Chris Dumez]

Comment on attachment 204996 [details]
Patch

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

r=me with nit.

> Tools/MiniBrowser/efl/main.c:443
> +        char *save_file_name = show_file_entry_dialog(window);

Eina_Stringshare *save_file_name =

[Santosh Mahto]

Created attachment 205003 [details]
Patch for Landing

[Ryuan Choi]

Comment on attachment 205003 [details]
Patch for Landing

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

Althogh basic operaion looks fine, but I hope that we have more kind example for lazy people like me.

> Tools/MiniBrowser/efl/main.c:361
> +    ef = eet_open(fileName, EET_FILE_MODE_WRITE);
> +    if (!ef)
> +        return;

What about print some error message? I spend some time to know why it does'nt work. :)

And, can we improve more for someone who may miss .mht extension?
It looks failed to load file which saved without .mht extension.

> Tools/MiniBrowser/efl/main.c:998
> +    elm_fileselector_entry_path_set(fs_entry, "/home");

Can we check getenv("HOME") like file on_file_chooser_request ?

[Chris Dumez]

(In reply to comment #24)
> (From update of attachment 205003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205003&action=review
> 
> Althogh basic operaion looks fine, but I hope that we have more kind example for lazy people like me.
> 
> > Tools/MiniBrowser/efl/main.c:361
> > +    ef = eet_open(fileName, EET_FILE_MODE_WRITE);
> > +    if (!ef)
> > +        return;
> 
> What about print some error message? I spend some time to know why it does'nt work. :)
> 
> And, can we improve more for someone who may miss .mht extension?
> It looks failed to load file which saved without .mht extension.
> 
> > Tools/MiniBrowser/efl/main.c:998
> > +    elm_fileselector_entry_path_set(fs_entry, "/home");
> 
> Can we check getenv("HOME") like file on_file_chooser_request ?

I like and agree with Ryuan's feedback :)

[Gyuyoung Kim]

Comment on attachment 205003 [details]
Patch for Landing

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

> Tools/MiniBrowser/efl/main.c:1025
> +    Eina_Stringshare *file_path = ok ? eina_stringshare_add(elm_fileselector_entry_path_get(fs_entry)) : NULL;

I think it would be better to keep default path or previous path instead of setting NULL when cancel button is pressed.

[Gyuyoung Kim]

Comment on attachment 205003 [details]
Patch for Landing

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

>> Tools/MiniBrowser/efl/main.c:998
>> +    elm_fileselector_entry_path_set(fs_entry, "/home");
> 
> Can we check getenv("HOME") like file on_file_chooser_request ?

One suggestion : When ctrl+s is pressed, we can get title of current web page using ewk_view_title_get(). So, if we set the title as default into input field, it would be nicer than now. I don't mind to support this suggestion on other bug. :)

[Santosh Mahto]

Created attachment 205169 [details]
Patch

[Santosh Mahto]

gyuyoung
>>One suggestion : When ctrl+s is pressed, we can get title of current web page >>using ewk_view_title_get().
Done..  default title will be "/path/Title.mht"

>>think it would be better to keep default path or previous path instead of >>setting NULL when cancel button is pressed

already done later by NULL check , I mean when new selected path is NULL then dont do save page operation 
if (!open_file_name)
       return;

ryuan
>>can we improve more for someone who may miss .mht extension?
making default text as "/path/Title.mht"  will do the job

[Chris Dumez]

Comment on attachment 205169 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:354
> +static void page_contents_callback(Ewk_Page_Contents_Type type, const char *data, void *user_data)

How about save_page_contents_callback?

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

Based on Ryuan's feedback, we should probably append '.mht' to save_file_name if it is missing.

> Tools/MiniBrowser/efl/main.c:999
> +    Evas_Object *fs_entry = elm_fileselector_entry_add(file_popup);

Shouldn't we distinguish loading and saving?
http://docs.enlightenment.org/auto/elementary/group__File__Selector__Entry.html#gaaf92afe71708d337d5d123565d2d2de3

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

show_file_entry_dialog() is generic, I don't think we should have mht specific stuff in there, please add function arguments instead if necessary.

[Santosh Mahto]

Created attachment 205175 [details]
Patch

[Santosh Mahto]

(In reply to comment #30)
> (From update of attachment 205169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205169&action=review
> 
> > Tools/MiniBrowser/efl/main.c:354
> > +static void page_contents_callback(Ewk_Page_Contents_Type type, const char *data, void *user_data)
> 
> How about save_page_contents_callback?
ok ..  done

> > Tools/MiniBrowser/efl/main.c:450
> > +        info("Save page (Ctrl + s) was pressed. Current page will be saved to %s\n", save_file_name);
> 
> Based on Ryuan's feedback, we should probably append '.mht' to save_file_name if it is missing.

Done

> > +    eina_strbuf_append_printf(default_file, "%s/%s.mht", getenv("HOME"), ewk_view_title_get(window->ewk_view));
> 
> show_file_entry_dialog() is generic, I don't think we should have mht specific stuff in there, please add function arguments instead if necessary.
Hmm.. you are right
I added a Label to distinguish the fileEntry type and also default text in entry based on new arg

[Chris Dumez]

Comment on attachment 205175 [details]
Patch

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#ga1e84de127ac0214f4983daeea3c3abf8

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

[Mikhail Pozdnyakov]

Comment on attachment 205175 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:184
> +static Eina_Stringshare *show_file_entry_dialog(Browser_Window *window, enum FileEntryType type);

why put 'enum' here?

> Tools/MiniBrowser/efl/main.c:1012
> +    char *label_tag = (type == Save) ? "SAVE" : "LOAD";

const char* ?

[Chris Dumez]

Comment on attachment 205175 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:184
>> +static Eina_Stringshare *show_file_entry_dialog(Browser_Window *window, enum FileEntryType type);
> 
> why put 'enum' here?

Because it is C, not C++? ;)

[Michal Pakula vel Rutka]

Comment on attachment 205175 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:62
> +enum FileEntryType { Save, Load };

I think we should follow EFL coding style in this file.

> Tools/MiniBrowser/efl/main.c:365
> +        info("Saving File: %s\n", eina_strbuf_string_get(fileNameWithMht));

info already adds new line, so please remove \n in all calls.

[Santosh Mahto]

Created attachment 205236 [details]
Patch

[Santosh Mahto]

@Michal Pakula vel Rutka
>> I think we should follow EFL coding style in this file.
 I think, we should follow webkit coding guidelines. otherwise buildbot style-check will fail.
>>info already adds new line, so please remove \n in all calls.
  hmm... i did

@Mikhail Pozdnyakov
>>> why put 'enum' here? 
  chris is correct, in C we need to use enum before type everywhere

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

This is really nice idea to make show_file_entry_dialog() as generic. initally i wasnot intended to do , but now modified it to be generic


> Tools/MiniBrowser/efl/main.c:1021
> +    elm_fileselector_entry_is_save_set(fs_entry, EINA_TRUE);
>> huh? What if type == LOAD?

elm_fileselector_entry_is_save_set is for enabling the entry textfiled and not related to save page, anyway now there is no LOAD/SAVE

[Chris Dumez]

Comment on attachment 205236 [details]
Patch

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

Looks better

> Tools/MiniBrowser/efl/main.c:461
> +        info("Save page (CTRL + S) was pressed. Current page will be saved to %s", eina_strbuf_string_get(default_file));

Still the same issue that this path may not be where we save the file since the show the file dialog after.  I think the "Current page will be saved to %s" part should be moved to the callback.

[Santosh Mahto]

(In reply to comment #39)
> (From update of attachment 205236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205236&action=review
> 
> Looks better
> 
> > Tools/MiniBrowser/efl/main.c:461
> > +        info("Save page (CTRL + S) was pressed. Current page will be saved to %s", eina_strbuf_string_get(default_file));
> 
> Still the same issue that this path may not be where we save the file since the show the file dialog after.  I think the "Current page will be saved to %s" part should be moved to the callback.

We already have Logs in callback  telling saved file path 
 info("Saving File: %s", eina_strbuf_string_get(fileNameWithMht));
 info("SUCCESS: saved.");

That  log  is meant to  tell user what action will be triggered by pressing  "CTRL + S"
I think i can put like this only for above
info("Pressed : (CTRL + S)  Saving Current Page.") 
it looks  enough, 
Actually only 3 kind of log is required 
1. CTRL +S --> specify action
2. specify Saved File Path
3. specify SUCESS

what you say ??

[Chris Dumez]

Comment on attachment 205236 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:363
> +        info("Saving File: %s", eina_strbuf_string_get(fileNameWithMht));

Saving file to

> Tools/MiniBrowser/efl/main.c:367
> +        info("Saving File: %s", fileName);

ditto.

>>> Tools/MiniBrowser/efl/main.c:461
>>> +        info("Save page (CTRL + S) was pressed. Current page will be saved to %s", eina_strbuf_string_get(default_file));
>> 
>> Still the same issue that this path may not be where we save the file since the show the file dialog after.  I think the "Current page will be saved to %s" part should be moved to the callback.
> 
> We already have Logs in callback  telling saved file path 
>  info("Saving File: %s", eina_strbuf_string_get(fileNameWithMht));
>  info("SUCCESS: saved.");
> 
> That  log  is meant to  tell user what action will be triggered by pressing  "CTRL + S"
> I think i can put like this only for above
> info("Pressed : (CTRL + S)  Saving Current Page.") 
> it looks  enough, 
> Actually only 3 kind of log is required 
> 1. CTRL +S --> specify action
> 2. specify Saved File Path
> 3. specify SUCESS
> 
> what you say ??

Right, you already print the path in the callback so let's remove it from here.

[Santosh Mahto]

Created attachment 205238 [details]
Patch

[Chris Dumez]

Comment on attachment 205238 [details]
Patch

LGTM. r=me but let Ryuan/Gyuyoung take a look before landing.

[Ryuan Choi]

Comment on attachment 205238 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:359
> +    if (!eina_str_has_suffix(fileName, ".mht")) {

IMO, eina_str_has_extension looks more sense.

[Santosh Mahto]

(In reply to comment #44)
> (From update of attachment 205238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205238&action=review
> 
> > Tools/MiniBrowser/efl/main.c:359
> > +    if (!eina_str_has_suffix(fileName, ".mht")) {
> 
> IMO, eina_str_has_extension looks more sense.

whats the advantage of using eina_str_has_extension 
Docs says: This function does the same as eina_str_has_suffix(), except it's case insensitive.
http://docs.enlightenment.org/auto/efl/group__Eina__String__Group.html#ga0671695663bd983c2b4af8a5fd867073

probably we want  case sensitive check

[Ryuan Choi]

(In reply to comment #45)
> (In reply to comment #44)
> > (From update of attachment 205238 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205238&action=review
> > 
> > > Tools/MiniBrowser/efl/main.c:359
> > > +    if (!eina_str_has_suffix(fileName, ".mht")) {
> > 
> > IMO, eina_str_has_extension looks more sense.
> 
> whats the advantage of using eina_str_has_extension 
> Docs says: This function does the same as eina_str_has_suffix(), except it's case insensitive.
> http://docs.enlightenment.org/auto/efl/group__Eina__String__Group.html#ga0671695663bd983c2b4af8a5fd867073
> 
> probably we want  case sensitive check

Right, only different with case insensitive.
IMO, It is for checking whether mht extension exist and I tested XXX.MHT works fine too.

[Chris Dumez]

Comment on attachment 205238 [details]
Patch

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

>>>> Tools/MiniBrowser/efl/main.c:359
>>>> +    if (!eina_str_has_suffix(fileName, ".mht")) {
>>> 
>>> IMO, eina_str_has_extension looks more sense.
>> 
>> whats the advantage of using eina_str_has_extension 
>> Docs says: This function does the same as eina_str_has_suffix(), except it's case insensitive.
>> http://docs.enlightenment.org/auto/efl/group__Eina__String__Group.html#ga0671695663bd983c2b4af8a5fd867073
>> 
>> probably we want  case sensitive check
> 
> Right, only different with case insensitive.
> IMO, It is for checking whether mht extension exist and I tested XXX.MHT works fine too.

Yes, I did not know about eina_str_has_extension(), it is better indeed. Thanks Ryuan!

@Santosh: please fix this nit before landing.

[Santosh Mahto]

Created attachment 205245 [details]
Patch

[Ryuan Choi]

Comment on attachment 205245 [details]
Patch

OK, Thanks.

Looks good to me.

[Chris Dumez]

Comment on attachment 205245 [details]
Patch

r+ again but you can carry over the review in such cases by updating the changelogs and only requesting cq

[WebKit Commit Bot]

Comment on attachment 205245 [details]
Patch

Clearing flags on attachment: 205245

Committed r151886: <http://trac.webkit.org/changeset/151886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.