Summary: | [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClientEfl should be based on C API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||
Component: | WebKit EFL | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, benjamin, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 107657, 107666 | ||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2013-01-23 06:08:00 PST
Created attachment 184222 [details]
patch
Comment on attachment 184222 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=184222&action=review > Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:72 > - ASSERT(context); > + ASSERT(m_soupRequestManager); where is this change explained? Comment on attachment 184222 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=184222&action=review >> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:72 >> + ASSERT(m_soupRequestManager); > > where is this change explained? ASSERT(context); was erroneous here, as context is used already before this line Comment on attachment 184222 [details]
patch
LGTM.
Comment on attachment 184222 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=184222&action=review Looks like a step in the right direction. Some comments: > Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:109 > -ContextHistoryClientEfl::ContextHistoryClientEfl(PassRefPtr<WebContext> context) > +ContextHistoryClientEfl::ContextHistoryClientEfl(WKContextRef context) I see you include DownloadProxy.h in this file, is that intentional? > Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.h:29 > +#include "WKRetainPtr.h" I would use #include <WebKit2/WKRetainPtr.h> to simplify the include paths. Then order the #includes. > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:29 > +#include "WKRetainPtr.h" Ditto. > Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:70 > -RequestManagerClientEfl::RequestManagerClientEfl(EwkContext* context) > - : m_soupRequestManager(WKContextGetSoupRequestManager(toAPI(context->webContext().get()))) > +RequestManagerClientEfl::RequestManagerClientEfl(WKContextRef context) > + : m_soupRequestManager(WKContextGetSoupRequestManager(context)) This class uses WebSoupRequestManagerProxy directly. It looks like it could easily switch to the C API. Comment on attachment 184222 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=184222&action=review >> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:109 >> +ContextHistoryClientEfl::ContextHistoryClientEfl(WKContextRef context) > > I see you include DownloadProxy.h in this file, is that intentional? apparently not :) thanks for the catch >> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.h:29 >> +#include "WKRetainPtr.h" > > I would use #include <WebKit2/WKRetainPtr.h> to simplify the include paths. Then order the #includes. sure it is better >> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:70 >> + : m_soupRequestManager(WKContextGetSoupRequestManager(context)) > > This class uses WebSoupRequestManagerProxy directly. It looks like it could easily switch to the C API. mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager' Created attachment 185419 [details]
patch v2
Took comments from Benjamin into consideration.
> mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager'
Here:
toImpl(m_soupRequestManager.get())->registerURIScheme(scheme);
(In reply to comment #8) > > mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager' > > Here: > toImpl(m_soupRequestManager.get())->registerURIScheme(scheme); indeed.. I haven't noticed :( Thanks for pointing out Created attachment 185498 [details]
patch v3
Removed remaining WK2 internal usage in RequestManagerClientEfl::registerURLSchemeHandler.
Comment on attachment 185498 [details]
patch v3
Looks good.
Just remove #include "WebSoupRequestManagerProxy.h" from RequestManagerClientEfl.cpp.
Committed r141389: <http://trac.webkit.org/changeset/141389> |