Bug 75246 - [WK2][EFL] creating dummy functions for Download class on Efl port.
Summary: [WK2][EFL] creating dummy functions for Download class on Efl port.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-27 04:39 PST by Keunsoon Lee
Modified: 2012-01-05 00:51 PST (History)
7 users (show)

See Also:


Attachments
initial implementation for download module specific to Efl port (23.59 KB, patch)
2011-12-28 03:33 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
fix style error (23.54 KB, patch)
2011-12-28 04:02 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
dummy functions for Download class for Efl port (5.08 KB, patch)
2012-01-03 02:18 PST, Keunsoon Lee
no flags Details | Formatted Diff | Diff
modified ChangeLog according to changed bug title (5.10 KB, patch)
2012-01-03 02:59 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 2011-12-27 04:39:55 PST
Added download module for Efl port on WebKit2/WebProcess/Downloads/efl.

Codes will be uploaded soon.
Comment 1 Keunsoon Lee 2011-12-28 03:33:59 PST
Created attachment 120648 [details]
initial implementation for download module specific to Efl port

This is initial version.
I'll try to make a test code if necessary.

EflDownloader is not dependent on Efl or soup port indeed, but uses only common classes on WebCore and WebKit2.
Efl port is using soup port for Network/HTTP stack, though.

PS, there is a known issue; EflDownloader::start() cannot obtain UserAgent when it is called from WKContextDownloadURLRequest() because it does not pass WebPage.
I'll create another bug thread for it.

Thank you.
Comment 2 WebKit Review Bot 2011-12-28 03:37:14 PST
Attachment 120648 [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/WebProcess/Downloads/efl/EflDownloader.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/Downloads/efl/EflDownloader.h:85:  The parameter name "download" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Keunsoon Lee 2011-12-28 04:02:47 PST
Created attachment 120650 [details]
fix style error

I leaved the style errors because it is a kind of style from another files on Source/WebKit2/WebProcess/Downloads.
But, I fixed them according to check-webkit-style script.
Comment 4 Gyuyoung Kim 2011-12-28 05:20:02 PST
Comment on attachment 120650 [details]
fix style error

View in context: https://bugs.webkit.org/attachment.cgi?id=120650&action=review

I think this patch is too huge to review. I think you need to make a patch which has dummy functions. Then, you submit concrete patches step by step. In addition, this patch doesn't include any test cases. Informal r- on my side.

> Source/WebKit2/ChangeLog:6
> +        newly implemented download module for Efl port

This description is not sufficient. I think you need to add more detail description ?

> Source/WebKit2/WebProcess/Downloads/efl/EflDownloader.cpp:44
> +    , m_bufferCapacity(64*1024) // FIXME: find proper value or give a chance to adjust it for client

Do place spaces around binary and ternary operators.

> Source/WebKit2/WebProcess/Downloads/efl/EflDownloader.cpp:66
> +            userAgent = "Mozilla/5.0 (Unknown; Linux armv7l) AppleWebKit/534.16+ (KHTML, like Gecko) Version/5.0 Safari/534.16+";

Why do you add armv7 as default ? In WebKit1, we make user agent string according to platform information and os version information. See ewk_settings.cpp files.

> Source/WebKit2/WebProcess/Downloads/efl/EflDownloader.cpp:71
> +    NetworkingContext* context = m_initiatingPage?m_initiatingPage->mainWebFrame()->coreFrame()->loader()->networkingContext():0;

ditto. Add space between ? : and so on.

> Source/WebKit2/WebProcess/Downloads/efl/EflDownloader.cpp:109
> +            || !destination.contains(PlatformFilePathSeparator)) {

Coding style violation.

 if (destination.isEmpty()
     || !destination.contains(PlatformFilePathSeparator)) {
Comment 5 Keunsoon Lee 2012-01-03 02:18:07 PST
Created attachment 120920 [details]
dummy functions for Download class for Efl port

This patch has only dummy functions for Download class because previous patch was too big to review.
I'll upload working patches in another bug threads.

Thank you in advance.
Comment 6 Gyuyoung Kim 2012-01-03 02:21:18 PST
Comment on attachment 120920 [details]
dummy functions for Download class for Efl port

LGTM. :-)
Comment 7 Gyuyoung Kim 2012-01-03 02:23:30 PST
(In reply to comment #6)
> (From update of attachment 120920 [details])
> LGTM. :-)

Would you change this Bug title according to your new patch ? If you change bug title, you have to change it in ChangeLog as well.
Comment 8 Keunsoon Lee 2012-01-03 02:59:21 PST
Created attachment 120924 [details]
modified ChangeLog according to changed bug title

Modified this bug's title and depending ChangeLog to emphasize dummy functions.
Comment 9 Gyuyoung Kim 2012-01-03 03:25:38 PST
LGTM.
Comment 10 Hajime Morrita 2012-01-04 22:12:37 PST
Comment on attachment 120924 [details]
modified ChangeLog according to changed bug title

rs=me
Comment 11 WebKit Review Bot 2012-01-05 00:51:17 PST
Comment on attachment 120924 [details]
modified ChangeLog according to changed bug title

Clearing flags on attachment: 120924

Committed r104122: <http://trac.webkit.org/changeset/104122>
Comment 12 WebKit Review Bot 2012-01-05 00:51:23 PST
All reviewed patches have been landed.  Closing bug.