Bug 76171

Created attachment 122220 [details] practical class to file download, which communicates with Download class

Attachment 122220 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 122222 [details] add bug number on ChangeLog

Comment on attachment 122222 [details] add bug number on ChangeLog r- for now to clear the review queue - It looks this patch met some unfortunate conflict. Please update to ToT.

Created attachment 122719 [details] change class name for paractical operation to DownloadEflImpl Uploaded new patch. A name of practical class to operate download is changed from EflDownloader to DownloadEflImpl. Because DownloadEfl and EflDownloader may cause confusion what they do.

Comment on attachment 122719 [details] change class name for paractical operation to DownloadEflImpl View in context: https://bugs.webkit.org/attachment.cgi?id=122719&action=review > Source/WebKit2/PlatformEfl.cmake:48 > + WebProcess/Downloads/efl/DownloadEflImpl.cpp I think Impl.cpp postfix is used by chromium port. So, it can make us confuse. In addition, DownloadEflImpl.cpp is not clear file name. In QT port cases, they add QtFileDownload.cpp. I think we also need to use more clear file name.

(In reply to comment #6) > (From update of attachment 122719 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122719&action=review > > > Source/WebKit2/PlatformEfl.cmake:48 > > + WebProcess/Downloads/efl/DownloadEflImpl.cpp > > I think Impl.cpp postfix is used by chromium port. So, it can make us confuse. In addition, DownloadEflImpl.cpp is not clear file name. In QT port cases, they add QtFileDownload.cpp. I think we also need to use more clear file name. Thank you for your review. And thank you for let me know about chromium port's naming rule. Several people who do not know about WebKit2 Download architecture asked me what EflDownloader is different from DownloadEfl. That is why I changed. But, I agree with you that the practical class should have port name prefix because it has its own header file. So, I will change it to EflFileDownloader like QT port, because its purpose is same with QTFileDownloader. Thank you.

Created attachment 124256 [details] Patch

Comment on attachment 124256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124256&action=review It seems to me FileDownloaderEfl is more proper name. Almost files add port name as postfix. > Source/WebKit2/WebProcess/Downloads/Download.h:158 > + OwnPtr<EflFileDownloader> m_eflFileDownloader; You already use #if PLATFORM(EFL). So, IMHO, you don't need to add efl prefix to variable name. I don't see port name prefix from now except for m_qtDownloader. m_eflFileDownloader -> m_fileDownloader;

Created attachment 125764 [details] Patch

Comment on attachment 125764 [details] Patch LGTM. :-)

Comment on attachment 125764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125764&action=review > Source/WebKit2/ChangeLog:7 > + It communicates with Download class to be ordered to start download Nit: you can omit "to be ordered" > Source/WebKit2/ChangeLog:10 > + Besides FileDownloaderEfl is a derived class from ResourceHandleClient. > + So, it can receive chunked data from ResourceHandle directly You can just say "FileDownloaderEfl derives from ResourceHandleClient and recieves chunked data"... > Source/WebKit2/ChangeLog:17 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description. > Source/WebKit2/WebProcess/Downloads/efl/FileDownloaderEfl.h:53 > + virtual void didReceiveData(WebCore::ResourceHandle*, const char*, int, int /*encodedDataLength*/); WebKit convention is to give argument variable names if the purpose of variables aren't self-evident as they're case for const char*, int, and int here instead of adding inline comment like this for annotation purposes. > Source/WebKit2/WebProcess/Downloads/efl/FileDownloaderEfl.h:54 > + virtual void didFinishLoading(WebCore::ResourceHandle*, double /*finishTime*/); Ditto.

Created attachment 125776 [details] Patch

Modified ChangeLog according to guide. And, added OVERRIDE at the end of all virtual methods' declaration on FileDownloaderEfl.h. Thank you.

(In reply to comment #12) > > Source/WebKit2/WebProcess/Downloads/efl/FileDownloaderEfl.h:53 > > + virtual void didReceiveData(WebCore::ResourceHandle*, const char*, int, int /*encodedDataLength*/); > > WebKit convention is to give argument variable names if the purpose of variables aren't self-evident as they're case for const char*, int, and int here > instead of adding inline comment like this for annotation purposes. Rniwa, I'm sorry for missing this nit. I won't miss similar nits in next time.

(In reply to comment #15) > (In reply to comment #12) > > > > Source/WebKit2/WebProcess/Downloads/efl/FileDownloaderEfl.h:53 > > > + virtual void didReceiveData(WebCore::ResourceHandle*, const char*, int, int /*encodedDataLength*/); > > > > WebKit convention is to give argument variable names if the purpose of variables aren't self-evident as they're case for const char*, int, and int here > > instead of adding inline comment like this for annotation purposes. > > Rniwa, I'm sorry for missing this nit. I won't miss similar nits in next time. Please make ~FileDownloaderEfl() destructor virtual.

Created attachment 125784 [details] Patch

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #12) > > > > > > Source/WebKit2/WebProcess/Downloads/efl/FileDownloaderEfl.h:53 > > > > + virtual void didReceiveData(WebCore::ResourceHandle*, const char*, int, int /*encodedDataLength*/); > > > > > > WebKit convention is to give argument variable names if the purpose of variables aren't self-evident as they're case for const char*, int, and int here > > > instead of adding inline comment like this for annotation purposes. > > > > Rniwa, I'm sorry for missing this nit. I won't miss similar nits in next time. > > Please make ~FileDownloaderEfl() destructor virtual. I fixed it. Thank you.

Comment on attachment 125784 [details] Patch Clearing flags on attachment: 125784 Committed r107180: <http://trac.webkit.org/changeset/107180>

All reviewed patches have been landed. Closing bug.