Bug 76171 – [WK2][EFL] creating dummy class derived from ResourceHandleClient for Efl download module.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Keunsoon Lee]

Created EflDownloader class, which is a practical class to operate file download.

This class communicates with Download class to be ordered to start download.
And, it is derived from ResourceHandleClient to redirect data flow for ResourceHandle from MainResourceLoader.

This patch has only dummy functions to make easy to review.
Working patch will be uploaded on another bug thread.

Thank you.

[Keunsoon Lee]

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

[WebKit Review Bot]

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.

[Keunsoon Lee]

Created attachment 122222 [details]
add bug number on ChangeLog

[Hajime Morrita]

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.

[Keunsoon Lee]

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.

[Gyuyoung Kim]

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.

[Keunsoon Lee]

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

[Keunsoon Lee]

Created attachment 124256 [details]
Patch

[Gyuyoung Kim]

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;

[Keunsoon Lee]

Created attachment 125764 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 125764 [details]
Patch

LGTM. :-)

[Ryosuke Niwa]

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.

[Keunsoon Lee]

Created attachment 125776 [details]
Patch

[Keunsoon Lee]

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

[Gyuyoung Kim]

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

[Tomasz Morawski]

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

[Keunsoon Lee]

Created attachment 125784 [details]
Patch

[Keunsoon Lee]

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

[WebKit Review Bot]

Comment on attachment 125784 [details]
Patch

Clearing flags on attachment: 125784

Committed r107180: <http://trac.webkit.org/changeset/107180>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.