Bug 106620

Summary: [EFL] Add support for MHTML save/load feature to MiniBrowser
Product: WebKit Reporter: KwangYong Choi <ky0.choi>
Component: WebKit EFLAssignee: KwangYong Choi <ky0.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, santosh.mahto, santosh.ma, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106440, 106752    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for Landing
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description KwangYong Choi 2013-01-10 17:04:17 PST
Add support for MHTML save/load feature to MiniBrowser.
Comment 1 KwangYong Choi 2013-01-13 20:29:37 PST
Created attachment 182501 [details]
Patch
Comment 2 Gyuyoung Kim 2013-01-14 21:06:46 PST
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 ?
Comment 3 KwangYong Choi 2013-01-15 19:52:45 PST
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.
Comment 4 Gyuyoung Kim 2013-01-15 19:57:25 PST
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.
Comment 5 KwangYong Choi 2013-01-15 20:03:42 PST
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 
)
Comment 6 Gyuyoung Kim 2013-01-15 20:05:34 PST
(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.
Comment 7 Gyuyoung Kim 2013-01-16 01:03:09 PST
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).
Comment 8 KwangYong Choi 2013-01-16 20:25:35 PST
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".
Comment 9 Santosh Mahto 2013-06-16 01:01:40 PDT
Created attachment 204782 [details]
Patch
Comment 10 Santosh Mahto 2013-06-16 01:05:18 PDT
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
Comment 11 Gyuyoung Kim 2013-06-16 23:53:02 PDT
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 ?
Comment 12 Chris Dumez 2013-06-17 00:37:17 PDT
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
Comment 13 Santosh Mahto 2013-06-18 06:46:04 PDT
Created attachment 204908 [details]
Patch
Comment 14 Santosh Mahto 2013-06-18 06:50:03 PDT
> 
> > 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
Comment 15 Chris Dumez 2013-06-18 07:01:15 PDT
(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().
Comment 16 Chris Dumez 2013-06-18 07:12:27 PDT
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.
Comment 17 Santosh Mahto 2013-06-19 04:30:53 PDT
> 
> > 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
Comment 18 Santosh Mahto 2013-06-19 04:41:18 PDT
Created attachment 204988 [details]
Patch
Comment 19 Chris Dumez 2013-06-19 04:42:50 PDT
(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.
Comment 20 Chris Dumez 2013-06-19 04:50:51 PDT
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
Comment 21 Santosh Mahto 2013-06-19 05:57:51 PDT
Created attachment 204996 [details]
Patch
Comment 22 Chris Dumez 2013-06-19 06:14:46 PDT
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 =
Comment 23 Santosh Mahto 2013-06-19 07:14:41 PDT
Created attachment 205003 [details]
Patch for Landing
Comment 24 Ryuan Choi 2013-06-20 03:36:58 PDT
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 ?
Comment 25 Chris Dumez 2013-06-20 03:39:46 PDT
(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 :)
Comment 26 Gyuyoung Kim 2013-06-20 04:18:25 PDT
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.
Comment 27 Gyuyoung Kim 2013-06-20 04:41:17 PDT
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. :)
Comment 28 Santosh Mahto 2013-06-21 04:11:13 PDT
Created attachment 205169 [details]
Patch
Comment 29 Santosh Mahto 2013-06-21 04:18:39 PDT
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
Comment 30 Chris Dumez 2013-06-21 04:23:48 PDT
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.
Comment 31 Santosh Mahto 2013-06-21 06:25:34 PDT
Created attachment 205175 [details]
Patch
Comment 32 Santosh Mahto 2013-06-21 06:30:04 PDT
(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
Comment 33 Chris Dumez 2013-06-21 06:50:42 PDT
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.
Comment 34 Mikhail Pozdnyakov 2013-06-21 07:06:08 PDT
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* ?
Comment 35 Chris Dumez 2013-06-21 07:16:11 PDT
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++? ;)
Comment 36 Michal Pakula vel Rutka 2013-06-21 07:26:50 PDT
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.
Comment 37 Santosh Mahto 2013-06-21 23:23:39 PDT
Created attachment 205236 [details]
Patch
Comment 38 Santosh Mahto 2013-06-21 23:34:14 PDT
@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
Comment 39 Chris Dumez 2013-06-22 00:42:32 PDT
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.
Comment 40 Santosh Mahto 2013-06-22 01:01:34 PDT
(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 ??
Comment 41 Chris Dumez 2013-06-22 01:06:46 PDT
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.
Comment 42 Santosh Mahto 2013-06-22 01:19:46 PDT
Created attachment 205238 [details]
Patch
Comment 43 Chris Dumez 2013-06-22 01:31:47 PDT
Comment on attachment 205238 [details]
Patch

LGTM. r=me but let Ryuan/Gyuyoung take a look before landing.
Comment 44 Ryuan Choi 2013-06-22 03:38:14 PDT
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.
Comment 45 Santosh Mahto 2013-06-22 04:04:04 PDT
(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
Comment 46 Ryuan Choi 2013-06-22 04:36:17 PDT
(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.
Comment 47 Chris Dumez 2013-06-22 05:11:11 PDT
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.
Comment 48 Santosh Mahto 2013-06-22 05:15:38 PDT
Created attachment 205245 [details]
Patch
Comment 49 Ryuan Choi 2013-06-22 05:36:31 PDT
Comment on attachment 205245 [details]
Patch

OK, Thanks.

Looks good to me.
Comment 50 Chris Dumez 2013-06-22 05:37:57 PDT
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
Comment 51 WebKit Commit Bot 2013-06-22 23:14:41 PDT
Comment on attachment 205245 [details]
Patch

Clearing flags on attachment: 205245

Committed r151886: <http://trac.webkit.org/changeset/151886>
Comment 52 WebKit Commit Bot 2013-06-22 23:14:47 PDT
All reviewed patches have been landed.  Closing bug.