Bug 86343 – [EFL] New signals in ewk_view to enable global history delegate functionality

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

[Mikhail Pozdnyakov]

New signals are needed to be attached into ewk_view to enable global history delegate functionality for EFL port( and unskip http/tests/globalhistory testcases). 
Global history delegate is an interface for WebKit clients to manage their own global history store.

The new signals should do following:
1) report that view did navigation and give the navigation details.
2) report that view performed a client redirect and give source and destination uris.
3) report that view performed a server redirect and give source and destination uris.
4) report that client should fill the visited links set.

[Mikhail Pozdnyakov]

Created attachment 141677 [details]
patch

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

Created attachment 141682 [details]
patch v2 (corrected due to efl style)

[Mikhail Pozdnyakov]

(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.

[Gyuyoung Kim]

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 ?

[Mikhail Pozdnyakov]

(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).

[Gyuyoung Kim]

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?

[Mikhail Pozdnyakov]

(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.

[Mikhail Pozdnyakov]

> 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.

[Gyuyoung Kim]

(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.

[Mikhail Pozdnyakov]

Created attachment 141722 [details]
patch v3 (removed "populate,visited,links" signal)

[Mikhail Pozdnyakov]

Created attachment 141723 [details]
patch v4 (removed "populate,visited,links" signal + changelog clean up)

[Mikhail Pozdnyakov]

(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.

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

Created attachment 141876 [details]
patch v5 (changelog clean up)

[Mikhail Pozdnyakov]

(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.

[Gyuyoung Kim]

Comment on attachment 141876 [details]
patch v5 (changelog clean up)

Looks good to me.

[Kenneth Rohde Christiansen]

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 ?

[Mikhail Pozdnyakov]

Created attachment 144536 [details]
patch v6 (Review remarks are met)

[Mikhail Pozdnyakov]

(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.

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

Created attachment 145057 [details]
patch v7 (Review comments are met. Thanks Chris)

[Mikhail Pozdnyakov]

Created attachment 145062 [details]
patch v7 (Review comments are met. Thanks Chris)

[Chris Dumez]

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".

[Mikhail Pozdnyakov]

Created attachment 145078 [details]
patch v8 (Removed typos from changelog)

[Chris Dumez]

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"...

[Mikhail Pozdnyakov]

Created attachment 145249 [details]
patch v9 (Another review remarks met. Chris thanks again)

[Chris Dumez]

Comment on attachment 145249 [details]
patch v9 (Another review remarks met. Chris thanks again)

Looks great.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.