Bug 91602

Summary:

[EFL][WK2] EFL should use DownloadSoup instead of defining DownloadEfl

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

d-r, gustavo, gyuyoung.kim, kenneth, keunsoon.lee, rakuco, tonikitoo, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

91598

Bug Blocks:

61838, 91345

Attachments:

Description	Flags
Patch	gustavo: commit-queue-
Patch	none
Patch	kenneth: review+, cdumez: commit-queue-
Patch for landing	none

Chris Dumez

Reported 2012-07-18 01:18:36 PDT

The EFL port is using libsoup and therefore we should reuse DownloadSoup.cpp in WK2-EFL instead of defining our own (empty) DownloadEfl.cpp. The only issue with that is that currently DownloadSoup.cpp includes "ErrorsGtk.h". We need to fix that.

Attachments
Patch (23.86 KB, patch) 2012-07-18 04:51 PDT, Chris Dumez	gustavo: commit-queue-	Details Formatted Diff Diff
Patch (24.51 KB, patch) 2012-07-18 05:25 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (24.51 KB, patch) 2012-07-18 05:45 PDT, Chris Dumez	kenneth: review+ cdumez: commit-queue-	Details Formatted Diff Diff
Patch for landing (24.34 KB, patch) 2012-07-18 13:10 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Keunsoon Lee

Comment 1 2012-07-18 03:36:28 PDT

I don't think we need to use DownloadSoup.cpp instead of DownloadEfl.cpp on WebKit2/WebProcess/Downloads/efl. Because Download itself should not depend on Loader or Soup port. By the way, I'm going to upstream the core of DownloadEfl.cpp soon. (https://bugs.webkit.org/show_bug.cgi?id=88077) What do you think so?

Chris Dumez

Comment 2 2012-07-18 03:44:29 PDT

Actually it depend(In reply to comment #1) > I don't think we need to use DownloadSoup.cpp instead of DownloadEfl.cpp on WebKit2/WebProcess/Downloads/efl. > Because Download itself should not depend on Loader or Soup port. > > By the way, I'm going to upstream the core of DownloadEfl.cpp soon. > (https://bugs.webkit.org/show_bug.cgi?id=88077) > > What do you think so? Actually, it depends on soup for the response (it is a SoupMessage). Otherwise, why would GTK port put the implementation in a soup folder instead of GTK. I don't want to duplicate the code in EFL for no reason. We are using soup and there is a DownloadSoup, therefore we should use it. My patch to use DownloadSoup in WebKit2-EFL is almost ready and it is small. I think it is the way to go.

Chris Dumez

Comment 3 2012-07-18 04:51:52 PDT

Created attachment 152987 [details] Patch

Gustavo Noronha (kov)

Comment 4 2012-07-18 05:02:22 PDT

Comment on attachment 152987 [details] Patch Attachment 152987 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13284469

Chris Dumez

Comment 5 2012-07-18 05:25:51 PDT

Created attachment 152996 [details] Patch Attempt to make gtk-ews happy.

Gyuyoung Kim

Comment 6 2012-07-18 05:35:24 PDT

(In reply to comment #1) > I don't think we need to use DownloadSoup.cpp instead of DownloadEfl.cpp on WebKit2/WebProcess/Downloads/efl. > Because Download itself should not depend on Loader or Soup port. > > By the way, I'm going to upstream the core of DownloadEfl.cpp soon. > (https://bugs.webkit.org/show_bug.cgi?id=88077) > > What do you think so? Keunsoon, do you have any features DownloadSoup can't support ? Basically, this is a way to go I think. So, I'd like to know if there is any reasons.

Chris Dumez

Comment 7 2012-07-18 05:45:41 PDT

Created attachment 152998 [details] Patch Fix a small typo.

Kenneth Rohde Christiansen

Comment 8 2012-07-18 11:18:37 PDT

Comment on attachment 152998 [details] Patch good point, rs=me

Chris Dumez

Comment 9 2012-07-18 11:50:41 PDT

Comment on attachment 152998 [details] Patch Clearing cq flag for now as I need the dependency to land first.

Chris Dumez

Comment 10 2012-07-18 13:10:52 PDT

Created attachment 153073 [details] Patch for landing Rebase on master now that the dependency landed. Could someone please cq+ ?

WebKit Review Bot

Comment 11 2012-07-18 14:31:48 PDT

Comment on attachment 153073 [details] Patch for landing Clearing flags on attachment: 153073 Committed r123019: <http://trac.webkit.org/changeset/123019>

WebKit Review Bot

Comment 12 2012-07-18 14:32:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.