Summary: | [EFL] Add central error management to EFL port | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | d-r, gyuyoung.kim, gyuyoung.kim, hausmann, jochen, kenneth, keunsoon.lee, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 61838, 91602 | ||||||||
Attachments: |
|
Description
Chris Dumez
2012-07-18 00:40:28 PDT
Created attachment 152966 [details]
Proposed patch
Comment on attachment 152966 [details]
Proposed patch
Looks good to me. Thanks !!
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 Created attachment 152975 [details]
Patch
Take Dominik and Jochen's feedback into consideration.
The patch looks good to me 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? Simon would it make sense to use some of this for Qt? (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. (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
> 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.
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. 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? Comment on attachment 152975 [details]
Patch
sure r=me
Could someone please cq+ ? I need this to land so that I can rebase the dependency at Bug 91602. Comment on attachment 152975 [details] Patch Clearing flags on attachment: 152975 Committed r123004: <http://trac.webkit.org/changeset/123004> All reviewed patches have been landed. Closing bug. |