We need default errors for wk1 and wk2, let's move them to a common spot.
Not sure about the refactoring, error codes might differ - let's put an implementation for Wk2 at least.
With error implementations in place, we're down to about 400 failures compared to DRT.
Created attachment 151210 [details] Patch
(In reply to comment #3) > Created an attachment (id=151210) [details] > Patch This patch creates meaningful ResourceError for each case and return it.
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.
Created attachment 151229 [details] Patch
(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.
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
Created attachment 151233 [details] Patch
(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].
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.
(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.
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
Created attachment 151399 [details] Patch
(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.
Comment on attachment 151399 [details] Patch It seems this patch is based on QT port's implementation. LGTM now.
(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
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)
Created attachment 151434 [details] Patch
(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?
(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?
(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
Comment on attachment 151434 [details] Patch rubberstamping.
Comment on attachment 151434 [details] Patch Clearing flags on attachment: 151434 Committed r122322: <http://trac.webkit.org/changeset/122322>
All reviewed patches have been landed. Closing bug.
*** Bug 90683 has been marked as a duplicate of this bug. ***