Add support for MHTML save/load feature to MiniBrowser.
Created attachment 182501 [details] Patch
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 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 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 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 )
(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 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 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".
Created attachment 204782 [details] Patch
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 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 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
Created attachment 204908 [details] Patch
> > > 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
(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 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.
> > > 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
Created attachment 204988 [details] Patch
(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 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
Created attachment 204996 [details] Patch
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 =
Created attachment 205003 [details] Patch for Landing
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 ?
(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 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 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. :)
Created attachment 205169 [details] Patch
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 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.
Created attachment 205175 [details] Patch
(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 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 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 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 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.
Created attachment 205236 [details] Patch
@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 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.
(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 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.
Created attachment 205238 [details] Patch
Comment on attachment 205238 [details] Patch LGTM. r=me but let Ryuan/Gyuyoung take a look before landing.
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.
(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
(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 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.
Created attachment 205245 [details] Patch
Comment on attachment 205245 [details] Patch OK, Thanks. Looks good to me.
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 on attachment 205245 [details] Patch Clearing flags on attachment: 205245 Committed r151886: <http://trac.webkit.org/changeset/151886>
All reviewed patches have been landed. Closing bug.