Bug 91598 – [EFL] Add central error management to EFL port

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

[Chris Dumez]

We need a ErrorsEfl.h header in WebCore that defines all the possible errors used by EFL port. We can then use that header in both WebKit1 and WebKit2 EFL.
This is the approach used by the GTK port.

At the moment, the EFL port has no central error management and we use static constants defined here and there for error domains and codes.

[Chris Dumez]

Created attachment 152966 [details]
Proposed patch

[Gyuyoung Kim]

Comment on attachment 152966 [details]
Proposed patch

Looks good to me. Thanks !!

[Dominik Röttsches (drott)]

Comment on attachment 152966 [details]
Proposed patch

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

LGTM with the two nits.

> Source/WebCore/platform/efl/ErrorsEfl.h:52
> +// Sync'd with Mac's WebKit Errors.

Can you mention here where these are copied from, i.e. 
// copied from WebKit/Misc/WebKitErrors[Private].h

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:365
> +    if (!strcmp(error->domain, "WebKitNetworkError")) {

I think this translation deserves a comment explaining why it's done - matching test expectations etc.

Comment on attachment 152966 [details]
Proposed patch

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

> Source/WebCore/platform/efl/ErrorsEfl.cpp:41
> +                         request.url().string(), "Load request cancelled");

you don't need to wrap the line here and below

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:374
>      if (error->failing_url && *error->failing_url != '\0') {

no need for { } since the body is one line only now

[Chris Dumez]

Created attachment 152975 [details]
Patch

Take Dominik and Jochen's feedback into consideration.

The patch looks good to me

[Kenneth Rohde Christiansen]

Comment on attachment 152975 [details]
Patch

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

> Source/WebCore/platform/efl/ErrorsEfl.cpp:42
> +namespace WebCore {
> +
> +ResourceError cancelledError(const ResourceRequest& request)
> +{
> +    return ResourceError(errorDomainNetwork, NetworkErrorCancelled, request.url().string(), "Load request cancelled");
> +}
> +

I guess this is not really EFL related. Maybe it makes sense to share with Qt?

[Kenneth Rohde Christiansen]

Simon would it make sense to use some of this for Qt?

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 152975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152975&action=review
> 
> > Source/WebCore/platform/efl/ErrorsEfl.cpp:42
> > +namespace WebCore {
> > +
> > +ResourceError cancelledError(const ResourceRequest& request)
> > +{
> > +    return ResourceError(errorDomainNetwork, NetworkErrorCancelled, request.url().string(), "Load request cancelled");
> > +}
> > +
> 
> I guess this is not really EFL related. Maybe it makes sense to share with Qt?

In Qt port, you would probably want to use tr() for the strings. GTK port is using Gettext for example.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 152975 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152975&action=review
> > 
> > > Source/WebCore/platform/efl/ErrorsEfl.cpp:42
> > > +namespace WebCore {
> > > +
> > > +ResourceError cancelledError(const ResourceRequest& request)
> > > +{
> > > +    return ResourceError(errorDomainNetwork, NetworkErrorCancelled, request.url().string(), "Load request cancelled");
> > > +}
> > > +
> > 
> > I guess this is not really EFL related. Maybe it makes sense to share with Qt?
> 
> In Qt port, you would probably want to use tr() for the strings. GTK port is using Gettext for example.

That could easily be fixed with a macro in Platform.h or so

[Kenneth Rohde Christiansen]

> That could easily be fixed with a macro in Platform.h or so

hmm, maybe not gettext has to look thought the files and search for _ or something. Not sure how tr() works.

[Simon Hausmann]

Comment on attachment 152975 [details]
Patch

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

>>>> Source/WebCore/platform/efl/ErrorsEfl.cpp:42
>>>> +
>>> 
>>> I guess this is not really EFL related. Maybe it makes sense to share with Qt?
>> 
>> In Qt port, you would probably want to use tr() for the strings. GTK port is using Gettext for example.
> 
> That could easily be fixed with a macro in Platform.h or so

Sounds interesting. The macro part I'm not sure about, because for the purpose of translation the name of the "function" that encloses the translation matters.

[Chris Dumez]

We need this as soon as possible for our port. While integrating with Qt (and likely GTK) is an interesting idea, this may take time as we need to solve some issues such as localization and patch all the relevant code for those ports. 

Could we land this patch like as it is for now and discuss the possible code sharing with other ports in a separate thread?

[Kenneth Rohde Christiansen]

Comment on attachment 152975 [details]
Patch

sure r=me

[Chris Dumez]

Could someone please cq+ ? I need this to land so that I can rebase the dependency at Bug 91602.

[WebKit Review Bot]

Comment on attachment 152975 [details]
Patch

Clearing flags on attachment: 152975

Committed r123004: <http://trac.webkit.org/changeset/123004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.