Summary: | [EFL] New signals in ewk_view to enable global history delegate functionality | ||
---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> |
Component: | WebKit EFL | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | gyuyoung.kim, 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: | 82579 | ||
Attachments: |
Description
Mikhail Pozdnyakov
2012-05-14 01:29:50 PDT
Created attachment 141677 [details]
patch
Comment on attachment 141677 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=141677&action=review > Source/WebKit/efl/ewk/ewk_view.h:306 > + const char* url; /**< url for the history */ Move '*' pointer to variable side. > Source/WebKit/efl/ewk/ewk_view.h:307 > + const char* title; /**< title of the navigated page */ ditto. > Source/WebKit/efl/ewk/ewk_view.h:310 > + Eina_Bool hasSubstituteData; /**< data substitution flag */ We are using efl coding style for public header files. So, clientRedirectSource need to be changed. For example, has_substitute_data. > Source/WebKit/efl/ewk/ewk_view.h:311 > + const char* clientRedirectSource; /**< client redirect source url*/ ditto. Created attachment 141682 [details]
patch v2 (corrected due to efl style)
(In reply to comment #2) > (From update of attachment 141677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141677&action=review > > > Source/WebKit/efl/ewk/ewk_view.h:306 > > + const char* url; /**< url for the history */ > > Move '*' pointer to variable side. > > > Source/WebKit/efl/ewk/ewk_view.h:307 > > + const char* title; /**< title of the navigated page */ > > ditto. > > > Source/WebKit/efl/ewk/ewk_view.h:310 > > + Eina_Bool hasSubstituteData; /**< data substitution flag */ > > We are using efl coding style for public header files. So, clientRedirectSource need to be changed. For example, has_substitute_data. > > > Source/WebKit/efl/ewk/ewk_view.h:311 > > + const char* clientRedirectSource; /**< client redirect source url*/ > > ditto. Thanks for the review! Fixed. Comment on attachment 141682 [details] patch v2 (corrected due to efl style) View in context: https://bugs.webkit.org/attachment.cgi?id=141682&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:398 > + evas_object_smart_callback_call(m_view, "populate,visited,links", 0); I don't understand why we need to send this signal. Could you explain this ? (In reply to comment #5) > (From update of attachment 141682 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141682&action=review > > > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:398 > > + evas_object_smart_callback_call(m_view, "populate,visited,links", 0); > > I don't understand why we need to send this signal. Could you explain this ? From ./Source/WebCore/ChangeLog-2008-08-10: Added populateVisitedLinks function, used to fill the visited links set from the global history at application startup time. So this signal is send to the client when when webcore needs information about visited links from a persistent history storage (managed by the client). Comment on attachment 141682 [details] patch v2 (corrected due to efl style) View in context: https://bugs.webkit.org/attachment.cgi?id=141682&action=review >>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:398 >>> + evas_object_smart_callback_call(m_view, "populate,visited,links", 0); >> >> I don't understand why we need to send this signal. Could you explain this ? > > From ./Source/WebCore/ChangeLog-2008-08-10: > Added populateVisitedLinks function, used to fill the > visited links set from the global history at application startup time. > > So this signal is send to the client when when webcore needs information about visited links from a persistent history storage (managed by the client). If so, I wonder how to set visited link information from client. Does ewk already have _set() APIs? (In reply to comment #7) > (From update of attachment 141682 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141682&action=review > > >>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:398 > >>> + evas_object_smart_callback_call(m_view, "populate,visited,links", 0); > >> > >> I don't understand why we need to send this signal. Could you explain this ? > > > > From ./Source/WebCore/ChangeLog-2008-08-10: > > Added populateVisitedLinks function, used to fill the > > visited links set from the global history at application startup time. > > > > So this signal is send to the client when when webcore needs information about visited links from a persistent history storage (managed by the client). > > If so, I wonder how to set visited link information from client. Does ewk already have _set() APIs? As far as I know such api is not available on EFL port and this signal can be used in DRT tests only at the moment. > As far as I know such api is not available on EFL port and this signal can be used in DRT tests only at the moment.
I could add this api, but I would create a separate bug for it.
(In reply to comment #9) > > As far as I know such api is not available on EFL port and this signal can be used in DRT tests only at the moment. > > I could add this api, but I would create a separate bug for it. From functionality point of view, I think we have to add a pair of function. In this case, it seems you need to implement functions that client sets according to "populate,visited,links" signal. Or, I think you're able to file a bug for "populate,visited,links" signal. Created attachment 141722 [details]
patch v3 (removed "populate,visited,links" signal)
Created attachment 141723 [details]
patch v4 (removed "populate,visited,links" signal + changelog clean up)
(In reply to comment #10) > (In reply to comment #9) > > > As far as I know such api is not available on EFL port and this signal can be used in DRT tests only at the moment. > > > > I could add this api, but I would create a separate bug for it. > > From functionality point of view, I think we have to add a pair of function. In this case, it seems you need to implement functions that client sets according to "populate,visited,links" signal. Or, I think you're able to file a bug for "populate,visited,links" signal. I've removed "populate,visited,links" signal from the patch, the bug86370 will be responsible for both the signal and new API setting visited link information from the client. Comment on attachment 141723 [details] patch v4 (removed "populate,visited,links" signal + changelog clean up) View in context: https://bugs.webkit.org/attachment.cgi?id=141723&action=review Looks make sense except for my trivial comment. > Source/WebKit/efl/ChangeLog:10 > + 1) report that view did navigation and give the navigation details. I heard that webkit changelog sublates to use '-', '1), 2)' and so on. I'm not sure whether we can use 1) in ChangeLog. Created attachment 141876 [details]
patch v5 (changelog clean up)
(In reply to comment #14) > (From update of attachment 141723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141723&action=review > > Looks make sense except for my trivial comment. > > > Source/WebKit/efl/ChangeLog:10 > > + 1) report that view did navigation and give the navigation details. > > I heard that webkit changelog sublates to use '-', '1), 2)' and so on. I'm not sure whether we can use 1) in ChangeLog. Fixed. Comment on attachment 141876 [details]
patch v5 (changelog clean up)
Looks good to me.
Comment on attachment 141876 [details] patch v5 (changelog clean up) View in context: https://bugs.webkit.org/attachment.cgi?id=141876&action=review > Source/WebKit/efl/ChangeLog:9 > + The new ewk_view signals do following: do the following* > Source/WebKit/efl/ChangeLog:10 > + Report that view did navigation and give the navigation details. Report that a navigation happened within the view and ... > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:977 > + Ewk_View_Navigation_Data data = > + {urlForHistory.data(), title.data(), > + {originalURL.data(), firstParty.data(), httpMethod.data(), 0, m_frame, isMainFrameRequest}, // request > + {responseURL.data(), loader->response().httpStatusCode(), 0, mimeType.data()}, // response Not really webkit codign style > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:978 > + loader->substituteData().isValid(), // has substituted data Comments are supposed to start with capital letter and end with a punctuation mark > Source/WebKit/efl/ewk/ewk_view.h:75 > + * - "perform,client,redirect", const char **: reports that view performed a client redirect and gives source and destination uris. uri's? URI's ? Created attachment 144536 [details]
patch v6 (Review remarks are met)
(In reply to comment #18) > (From update of attachment 141876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141876&action=review > > > Source/WebKit/efl/ChangeLog:9 > > + The new ewk_view signals do following: > > do the following* > > > Source/WebKit/efl/ChangeLog:10 > > + Report that view did navigation and give the navigation details. > > Report that a navigation happened within the view and ... > > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:977 > > + Ewk_View_Navigation_Data data = > > + {urlForHistory.data(), title.data(), > > + {originalURL.data(), firstParty.data(), httpMethod.data(), 0, m_frame, isMainFrameRequest}, // request > > + {responseURL.data(), loader->response().httpStatusCode(), 0, mimeType.data()}, // response > > Not really webkit codign style > > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:978 > > + loader->substituteData().isValid(), // has substituted data > > Comments are supposed to start with capital letter and end with a punctuation mark > > > Source/WebKit/efl/ewk/ewk_view.h:75 > > + * - "perform,client,redirect", const char **: reports that view performed a client redirect and gives source and destination uris. > > uri's? URI's ? Thanks for your comments. Fixed. Comment on attachment 144536 [details] patch v6 (Review remarks are met) View in context: https://bugs.webkit.org/attachment.cgi?id=144536&action=review > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:519 > + const char* data[2] = {sourceURL.data(), destURL.data()}; I would define a Ewk struct for this. This is less bug-prone for the client. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:526 > + const char* data[2] = {sourceURL.data(), destURL.data()}; Ditto. > Source/WebKit/efl/ewk/ewk_view.h:75 > + * - "perform,client,redirect", const char **: reports that view performed a client redirect and gives source and destination URI's. Remove space before the *. Also minor typo: "URIs" > Source/WebKit/efl/ewk/ewk_view.h:76 > + * - "perform,server,redirect", const char **: reports that view performed a server redirect and gives source and destination URI's. Ditto. > Source/WebKit/efl/ewk/ewk_view.h:311 > + Ewk_Frame_Resource_Request request; /**< Navigation request. */ Couldn't we make this a pointer to avoid copying the struct? > Source/WebKit/efl/ewk/ewk_view.h:312 > + Ewk_Frame_Resource_Response response; /**< Navigation response. */ Ditto. Created attachment 145057 [details]
patch v7 (Review comments are met. Thanks Chris)
Created attachment 145062 [details]
patch v7 (Review comments are met. Thanks Chris)
Comment on attachment 145062 [details] patch v7 (Review comments are met. Thanks Chris) View in context: https://bugs.webkit.org/attachment.cgi?id=145062&action=review Otherwise, LGTM. > Source/WebKit/efl/ChangeLog:17 > + * ewk/ewk_view.h: New signals and new Ewk_View_Navigation_Data and Ewk_View_Redirectionion_Data types. "Ewk_View_Redirectionion_Data" <- typo, + extra "and". Created attachment 145078 [details]
patch v8 (Removed typos from changelog)
Comment on attachment 145078 [details] patch v8 (Removed typos from changelog) View in context: https://bugs.webkit.org/attachment.cgi?id=145078&action=review > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:518 > + const CString& destURL = loader->clientRedirectDestinationForHistory().utf8(); According to WebKit coding style, we should avoid abbreviations. -> destinationURL. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:525 > + const CString& destURL = loader->serverRedirectDestinationForHistory().utf8(); Ditto. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:1004 > + bool substituteData = loader->substituteData().isValid(); According to WebKit coding style, boolean variable names should start with words like "is", "did", "should", "has"... Created attachment 145249 [details]
patch v9 (Another review remarks met. Chris thanks again)
Comment on attachment 145249 [details]
patch v9 (Another review remarks met. Chris thanks again)
Looks great.
Comment on attachment 145249 [details] patch v9 (Another review remarks met. Chris thanks again) Clearing flags on attachment: 145249 Committed r119223: <http://trac.webkit.org/changeset/119223> All reviewed patches have been landed. Closing bug. |