Bug 76171 - [WK2][EFL] creating dummy class derived from ResourceHandleClient for Efl download module.
Summary: [WK2][EFL] creating dummy class derived from ResourceHandleClient for Efl dow...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-01-12 05:28 PST by Keunsoon Lee
Modified: 2012-02-08 22:13 PST (History)
5 users (show)

See Also:


Attachments
practical class to file download, which communicates with Download class (10.72 KB, patch)
2012-01-12 05:33 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
add bug number on ChangeLog (10.77 KB, patch)
2012-01-12 05:43 PST, Keunsoon Lee
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
change class name for paractical operation to DownloadEflImpl (11.19 KB, patch)
2012-01-16 23:39 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
Patch (11.51 KB, patch)
2012-01-26 21:57 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2012-02-06 21:45 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2012-02-07 00:13 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2012-02-07 01:38 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keunsoon Lee 2012-01-12 05:28:59 PST
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.
Comment 1 Keunsoon Lee 2012-01-12 05:33:02 PST
Created attachment 122220 [details]
practical class to file download, which communicates with Download class
Comment 2 WebKit Review Bot 2012-01-12 05:35:45 PST
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.
Comment 3 Keunsoon Lee 2012-01-12 05:43:18 PST
Created attachment 122222 [details]
add bug number on ChangeLog
Comment 4 Hajime Morrita 2012-01-16 02:05:57 PST
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.
Comment 5 Keunsoon Lee 2012-01-16 23:39:55 PST
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 6 Gyuyoung Kim 2012-01-17 01:04:03 PST
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.
Comment 7 Keunsoon Lee 2012-01-17 02:35:40 PST
(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.
Comment 8 Keunsoon Lee 2012-01-26 21:57:40 PST
Created attachment 124256 [details]
Patch
Comment 9 Gyuyoung Kim 2012-02-06 20:20:18 PST
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;
Comment 10 Keunsoon Lee 2012-02-06 21:45:10 PST
Created attachment 125764 [details]
Patch
Comment 11 Gyuyoung Kim 2012-02-06 21:53:55 PST
Comment on attachment 125764 [details]
Patch

LGTM. :-)
Comment 12 Ryosuke Niwa 2012-02-06 22:56:39 PST
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.
Comment 13 Keunsoon Lee 2012-02-07 00:13:13 PST
Created attachment 125776 [details]
Patch
Comment 14 Keunsoon Lee 2012-02-07 00:18:32 PST
Modified ChangeLog according to guide.
And, added OVERRIDE at the end of all virtual methods' declaration on FileDownloaderEfl.h.
Thank you.
Comment 15 Gyuyoung Kim 2012-02-07 00:23:26 PST
(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.
Comment 16 Tomasz Morawski 2012-02-07 00:32:42 PST
(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.
Comment 17 Keunsoon Lee 2012-02-07 01:38:45 PST
Created attachment 125784 [details]
Patch
Comment 18 Keunsoon Lee 2012-02-07 02:29:24 PST
(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 19 WebKit Review Bot 2012-02-08 22:13:02 PST
Comment on attachment 125784 [details]
Patch

Clearing flags on attachment: 125784

Committed r107180: <http://trac.webkit.org/changeset/107180>
Comment 20 WebKit Review Bot 2012-02-08 22:13:07 PST
All reviewed patches have been landed.  Closing bug.