Bug 90688

Summary:

[EFL][Wk2] WebErrorsEfl.cpp needs to return non-empty errors

Product:

WebKit

Reporter:

Dominik Röttsches (drott) <d-r>

Component:

WebKit EFL

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, gustavo, gyuyoung.kim, jochen, kenneth, keunsoon.lee, lucas.de.marchi, mrobinson, naginenis, pnormand, ryuan.choi, tmpsantos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

90788

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Dominik Röttsches (drott)

Reported 2012-07-06 07:12:24 PDT

We need default errors for wk1 and wk2, let's move them to a common spot.

Attachments
Patch (4.21 KB, patch) 2012-07-09 01:23 PDT, Keunsoon Lee	no flags	Details Formatted Diff Diff
Patch (4.23 KB, patch) 2012-07-09 04:09 PDT, Keunsoon Lee	no flags	Details Formatted Diff Diff
Patch (4.23 KB, patch) 2012-07-09 05:01 PDT, Keunsoon Lee	no flags	Details Formatted Diff Diff
Patch (4.37 KB, patch) 2012-07-09 22:12 PDT, Keunsoon Lee	no flags	Details Formatted Diff Diff
Patch (4.26 KB, patch) 2012-07-10 04:17 PDT, Keunsoon Lee	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Dominik Röttsches (drott)

Comment 1 2012-07-06 07:27:48 PDT

Not sure about the refactoring, error codes might differ - let's put an implementation for Wk2 at least.

Dominik Röttsches (drott)

Comment 2 2012-07-06 07:34:32 PDT

With error implementations in place, we're down to about 400 failures compared to DRT.

Keunsoon Lee

Comment 3 2012-07-09 01:23:27 PDT

Created attachment 151210 [details] Patch

Keunsoon Lee

Comment 4 2012-07-09 01:25:02 PDT

(In reply to comment #3) > Created an attachment (id=151210) [details] > Patch This patch creates meaningful ResourceError for each case and return it.

Gyuyoung Kim

Comment 5 2012-07-09 03:38:43 PDT

Comment on attachment 151210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151210&action=review > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:41 > + /* FIXME remove magic number -999 and string "NetworkErrorDomain". Use // instead of /* ... */. Then, use *FIXME:* > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:42 > + * application cannot understand those. s/application/Application/g > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 > + * should establish EFL port's error system */ Please write completed sentence. > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:74 > + /* FIXME remove magic number -998 and string "NetworkErrorDomain". ditto.

Keunsoon Lee

Comment 6 2012-07-09 04:09:28 PDT

Created attachment 151229 [details] Patch

Keunsoon Lee

Comment 7 2012-07-09 04:11:38 PDT

(In reply to comment #5) > (From update of attachment 151210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151210&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:41 > > + /* FIXME remove magic number -999 and string "NetworkErrorDomain". > > Use // instead of /* ... */. Then, use *FIXME:* > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:42 > > + * application cannot understand those. > > s/application/Application/g > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 > > + * should establish EFL port's error system */ > > Please write completed sentence. > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:74 > > + /* FIXME remove magic number -998 and string "NetworkErrorDomain". > > ditto. Hi, I attached modified patch below. Thank you for your review.

Gyuyoung Kim

Comment 8 2012-07-09 04:46:08 PDT

Comment on attachment 151229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151229&action=review > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:41 > + // FIXME remove magic number -999 and string "NetworkErrorDomain". Use *FIXME:*. s/remove/Remove/g

Keunsoon Lee

Comment 9 2012-07-09 05:01:39 PDT

Created attachment 151233 [details] Patch

Keunsoon Lee

Comment 10 2012-07-09 05:03:46 PDT

(In reply to comment #8) > (From update of attachment 151229 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151229&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:41 > > + // FIXME remove magic number -999 and string "NetworkErrorDomain". > > Use *FIXME:*. s/remove/Remove/g Sorry that I missed it. Please refer to another patch; Created an attachment (id=151233) [details].

Gyuyoung Kim

Comment 11 2012-07-09 05:09:04 PDT

Comment on attachment 151233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151233&action=review > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 > + // because application cannot understand those. Could you file a bug for this issue? Then, write the url to here as well.

Keunsoon Lee

Comment 12 2012-07-09 05:43:59 PDT

(In reply to comment #11) > (From update of attachment 151233 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151233&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 > > + // because application cannot understand those. > > Could you file a bug for this issue? Then, write the url to here as well. Sure. Here is the bug id for the FIXME; https://bugs.webkit.org/show_bug.cgi?id=90783 . Thanks again.

Gyuyoung Kim

Comment 13 2012-07-09 19:46:14 PDT

Comment on attachment 151233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151233&action=review >>> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 >>> + // because application cannot understand those. >> >> Could you file a bug for this issue? Then, write the url to here as well. > > Sure. > Here is the bug id for the FIXME; https://bugs.webkit.org/show_bug.cgi?id=90783 . > > Thanks again. I mean please mention the bug url to this FIXME: comment. Please see also below url. http://trac.webkit.org/browser/trunk/Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp#L80

Keunsoon Lee

Comment 14 2012-07-09 22:12:10 PDT

Created attachment 151399 [details] Patch

Keunsoon Lee

Comment 15 2012-07-09 22:13:16 PDT

(In reply to comment #13) > (From update of attachment 151233 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151233&action=review > > >>> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:43 > >>> + // because application cannot understand those. > >> > >> Could you file a bug for this issue? Then, write the url to here as well. > > > > Sure. > > Here is the bug id for the FIXME; https://bugs.webkit.org/show_bug.cgi?id=90783 . > > > > Thanks again. > > I mean please mention the bug url to this FIXME: comment. Please see also below url. > http://trac.webkit.org/browser/trunk/Source/WebKit/efl/WebCoreSupport/NetworkInfoClientEfl.cpp#L80 I uploaded new patch according to your guide; Created an attachment (id=151399). Thanks.

Gyuyoung Kim

Comment 16 2012-07-09 22:29:12 PDT

Comment on attachment 151399 [details] Patch It seems this patch is based on QT port's implementation. LGTM now.

Chris Dumez

Comment 17 2012-07-09 22:30:20 PDT

(In reply to comment #16) > (From update of attachment 151399 [details]) > It seems this patch is based on QT port's implementation. LGTM now. Qt :P

Chris Dumez

Comment 18 2012-07-09 22:33:41 PDT

Comment on attachment 151399 [details] Patch LGTM as a first patch.

jochen

Comment 19 2012-07-10 03:02:40 PDT

Comment on attachment 151399 [details] Patch Can you add tests for this change? View in context: https://bugs.webkit.org/attachment.cgi?id=151399&action=review > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:45 > + return ResourceError("NetworkErrorDomain", -999, what about defining the string NetworkErrorDomain as a constant, as it's used several times? And define -999 and -998 as constants as well, so at least in this file, they have meaningful names? > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:46 > + request.url().string(), "Request cancelled"); nit. should fit in one line (here and all other return statements in this change)

Keunsoon Lee

Comment 20 2012-07-10 04:17:53 PDT

Created attachment 151434 [details] Patch

Keunsoon Lee

Comment 21 2012-07-10 04:18:41 PDT

(In reply to comment #19) > (From update of attachment 151399 [details]) > Can you add tests for this change? > > View in context: https://bugs.webkit.org/attachment.cgi?id=151399&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:45 > > + return ResourceError("NetworkErrorDomain", -999, > > what about defining the string NetworkErrorDomain as a constant, as it's used several times? > > And define -999 and -998 as constants as well, so at least in this file, they have meaningful names? > > > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:46 > > + request.url().string(), "Request cancelled"); > > nit. should fit in one line (here and all other return statements in this change) Thanks for the review. I attached new patch for your review; attachment (id=151434) .

jochen

Comment 22 2012-07-10 04:50:00 PDT

The code looks good. Any reason you didn't add tests?

Keunsoon Lee

Comment 23 2012-07-10 05:26:51 PDT

(In reply to comment #22) > The code looks good. > > Any reason you didn't add tests? That's because already there is a test case for it. WTR is running now, and the pass rate will be improved with this patch. I'm sorry that I cannot show you the actual result.

jochen

Comment 24 2012-07-10 05:43:42 PDT

(In reply to comment #23) > (In reply to comment #22) > > The code looks good. > > > > Any reason you didn't add tests? > > That's because already there is a test case for it. > WTR is running now, and the pass rate will be improved with this patch. > I'm sorry that I cannot show you the actual result. Shouldn't you update some expectations from TEXT to PASS then?

Dominik Röttsches (drott)

Comment 25 2012-07-10 22:29:47 PDT

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > The code looks good. > > > > > > Any reason you didn't add tests? > > > > That's because already there is a test case for it. > > WTR is running now, and the pass rate will be improved with this patch. > > I'm sorry that I cannot show you the actual result. > > Shouldn't you update some expectations from TEXT to PASS then? This patch improves pass rate on the WTR / WebKit2 based tests - they are not run on a bot yet. So, this patch shortens the delta that there is currently between WebKit2/WTR and Webkit(1) based DRT - that's why we'd like to have it in. The relevant tests are already executed.

jochen

Comment 26 2012-07-11 00:57:39 PDT

Ok, thanks for the clarification This patch looks good to me then

Hajime Morrita

Comment 27 2012-07-11 02:20:12 PDT

Comment on attachment 151434 [details] Patch rubberstamping.

WebKit Review Bot

Comment 28 2012-07-11 02:54:40 PDT

Comment on attachment 151434 [details] Patch Clearing flags on attachment: 151434 Committed r122322: <http://trac.webkit.org/changeset/122322>

WebKit Review Bot

Comment 29 2012-07-11 02:54:47 PDT

All reviewed patches have been landed. Closing bug.

Dominik Röttsches (drott)

Comment 30 2012-08-02 07:36:31 PDT

*** Bug 90683 has been marked as a duplicate of this bug. ***

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