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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

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

[Keunsoon Lee]

Created attachment 151210 [details]
Patch

[Keunsoon Lee]

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

Created attachment 151229 [details]
Patch

[Keunsoon Lee]

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

Created attachment 151233 [details]
Patch

[Keunsoon Lee]

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

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

Created attachment 151399 [details]
Patch

[Keunsoon Lee]

(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 on attachment 151399 [details]
Patch

It seems this patch is based on QT port's implementation. LGTM now.

[Chris Dumez]

(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 on attachment 151399 [details]
Patch

LGTM as a first patch.

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]

Created attachment 151434 [details]
Patch

[Keunsoon Lee]

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

The code looks good.

Any reason you didn't add tests?

[Keunsoon Lee]

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

(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)]

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

Ok, thanks for the clarification

This patch looks good to me then

[Hajime Morrita]

Comment on attachment 151434 [details]
Patch

rubberstamping.

[WebKit Review Bot]

Comment on attachment 151434 [details]
Patch

Clearing flags on attachment: 151434

Committed r122322: <http://trac.webkit.org/changeset/122322>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

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