Bug 98594 – [EFL][WK2] Add History callbacks API

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

[Mikhail Pozdnyakov]

History Client API should be added to Ewk_Context so that it is possible for the API client to compose global history.
An important part of this API is visited links tracking.

[Mikhail Pozdnyakov]

Created attachment 167520 [details]
WIP patch

WIP as
- no unit test (currently working on them)
- no Change log records yet
- missing description for History Client callback parameters

But still the whole approach is already clear and I would like to hear opinions (remarks) about it.

[Gyuyoung Kim]

Comment on attachment 167520 [details]
WIP patch

Attachment 167520 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14196823

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 167520 [details]
WIP patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:117
> +        memset(&historyClient, 0, sizeof(Ewk_Context_History_Client));

Why is this necessary?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:301
> +    if (!client) {
> +        memset(&ewkContext->historyClient, 0, sizeof(Ewk_Context_History_Client));
> +        return;
> +    }
> +
> +    memcpy(&ewkContext->historyClient, client, sizeof(Ewk_Context_History_Client));

Why is this better than a simple assignment?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:304
> +const Ewk_Context_History_Client *ewk_context_history_client_get(const Ewk_Context* ewkContext)

Wrong placement of the first * (I feel like Gyuyoung after reading what I wrote :-)

> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data.h:28
> + * @brief   Describes the Ewk navigation data API.

It would also be nice to explain what an "EWK Navigation Data" is.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:86
> +static PageViewMap viewMap;

Creating a static non-POD object like this is usually dangerous, using DEFINE_STATIC_LOCAL is probably better.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1552
> +    return (found != viewMap.end()) ? found->second : 0;

You probably need to use `value' instead of `second' after http://trac.webkit.org/changeset/130612.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:97
> +const Evas_Object *get_veiw_by_page(const WebKit::WebPageProxy*);

Typo: veiw. Any reason for not using the usual naming scheme here, btw?

[Kenneth Rohde Christiansen]

Comment on attachment 167520 [details]
WIP patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:117
>> +        memset(&historyClient, 0, sizeof(Ewk_Context_History_Client));
> 
> Why is this necessary?

Remember memset is not a free operation

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:113
> +    Ewk_Context_Did_Navigate_With_Navigation_Data_Cb didNavigateWithNavigationData;
> +    Ewk_Context_Did_Perform_Client_Redirect_Cb didPerformClientRedirect;
> +    Ewk_Context_Did_Perform_Server_Redirect_Cb didPerformServerRedirect;
> +    Ewk_Context_Did_Update_History_Title_Cb didUpdateHistoryTitle;
> +    Ewk_Context_Populate_Visited_Links_Cb populateVisitedLinks;

I am wondering whether these should follow more EFL naming practices? Or is this only used internally?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:779
> +    addViewToMap(ewkView);

which map? We should use better func names

>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:97
>> +const Evas_Object *get_veiw_by_page(const WebKit::WebPageProxy*);
> 
> Typo: veiw. Any reason for not using the usual naming scheme here, btw?

isn't it from instead of by?

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 167520 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167520&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:117
> >> +        memset(&historyClient, 0, sizeof(Ewk_Context_History_Client));
> > 
> > Why is this necessary?
> 
I was under influence of WK Clients which have different sizes depending on version, so memset() is often used when working with them :)
Off course, default initialization in constructor is better to use in this case.
Thanks for pointing this out!

[Mikhail Pozdnyakov]

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:86
> > +static PageViewMap viewMap;
> 
> Creating a static non-POD object like this is usually dangerous, using DEFINE_STATIC_LOCAL is probably better.

Thanks for nice hint!

[Mikhail Pozdnyakov]

Created attachment 167547 [details]
patch

Added unit tests. Took comments from Rakuco and Kenneth into consideration.

[Gyuyoung Kim]

Comment on attachment 167547 [details]
patch

Attachment 167547 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14218333

[Mikhail Pozdnyakov]

Created attachment 167552 [details]
patch v2

uri -> url, test_ewk2_context_history_delegate.cpp -> test_ewk2_context_history_client.cpp

[Chris Dumez]

Comment on attachment 167552 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
> +        , historyClient()

Does this really initialize all the members to 0?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:296
> +        ewkContext->historyClient = { 0, 0, 0, 0, 0, 0 };

Why not use memset here? This be more maintainable when we add new callbacks.

> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data.cpp:107
> +    ASSERT(dataRef);

EINA_SAFETY_ON_NULL_RETURN_VAL?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:86
> +DEFINE_STATIC_LOCAL(PageViewMap, viewMap, ());

This is usually used inside a static function.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1550
> +    PageViewMap::const_iterator found = viewMap.find(page);

Why not simply "return viewMap.get(page);" ?

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:97
> +const Evas_Object *ewk_view_from_page_get(const WebKit::WebPageProxy*);

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_history_client.cpp:132
> +    (*countLoadFinished)--;

preincrement?

[Mikhail Pozdnyakov]

Comment on attachment 167552 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
>> +        , historyClient()
> 
> Does this really initialize all the members to 0?

yep, it is POD value initialization

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:296
>> +        ewkContext->historyClient = { 0, 0, 0, 0, 0, 0 };
> 
> Why not use memset here? This be more maintainable when we add new callbacks.

I do not believe, we'll ever add new callbacks to it, but I'm OK with memset

>> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data.cpp:107
>> +    ASSERT(dataRef);
> 
> EINA_SAFETY_ON_NULL_RETURN_VAL?

it's not public function and we really do not anticipate '0' here. I can add EINA_SAFETY_ON_NULL_RETURN_VAL but would keep the assertion as well

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:86
>> +DEFINE_STATIC_LOCAL(PageViewMap, viewMap, ());
> 
> This is usually used inside a static function.

my bad :(

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1550
>> +    PageViewMap::const_iterator found = viewMap.find(page);
> 
> Why not simply "return viewMap.get(page);" ?

Indeed! WTF::HashMap::get handles the case when key is not found.

[Mikhail Pozdnyakov]

Created attachment 167579 [details]
patch v3

Took comments from Chris into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 167579 [details]
patch v3

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:109
> +    Ewk_Context_Did_Navigate_With_Navigation_Data_Cb ewk_context_navigation_performed;

Is it really good that the callback def is so differently named than the func? Do you have examples of this from other EFL libs? (maybe it is all just inconsistent :-)) just wondering as I dont like changing api later

> Source/WebKit2/UIProcess/API/efl/ewk_context_history_client.cpp:133
> +
> +

why two newlines?

[Gyuyoung Kim]

Comment on attachment 167579 [details]
patch v3

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:310
> +void ewk_context_add_visited_link(Ewk_Context* ewkContext, const char* visitedURL)

Generally, *_add* is placed at the end of function on efl API side.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1552
> +const Evas_Object* ewk_view_from_page_get(const WebKit::WebPageProxy* page)

Just wondering, do you think this function name is reasonable ?  ewk_view_get? or ewk_view_get_from_page(because this is private function) ?

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (From update of attachment 167579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167579&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:310
> > +void ewk_context_add_visited_link(Ewk_Context* ewkContext, const char* visitedURL)
> 
> Generally, *_add* is placed at the end of function on efl API side.

yes, thanks for catch!

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1552
> > +const Evas_Object* ewk_view_from_page_get(const WebKit::WebPageProxy* page)
> 
> Just wondering, do you think this function name is reasonable ?  ewk_view_get? or ewk_view_get_from_page(because this is private function) ?

Even though it's private API I would keep it in consistence with public API names. 'ewk_view_get' to my mind would be too generic.

[Mikhail Pozdnyakov]

(In reply to comment #13)
> (From update of attachment 167579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167579&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:109
> > +    Ewk_Context_Did_Navigate_With_Navigation_Data_Cb ewk_context_navigation_performed;
> 
> Is it really good that the callback def is so differently named than the func? Do you have examples of this from other EFL libs? (maybe it is all just inconsistent :-)) just wondering as I dont like changing api later
> 

Yeah, it looks quite ugly.. I should think of renaming maybe both defs and func names.

[Mikhail Pozdnyakov]

Created attachment 167728 [details]
patch v4

Having a discussion with Kenneth came to conclusion that having "set callbacks" function in public API is more appropriate from EFL style POV.

[Mikhail Pozdnyakov]

(In reply to comment #17)
> Created an attachment (id=167728) [details]
> patch v4
> 
> Having a discussion with Kenneth came to conclusion that having "set callbacks" function in public API is more appropriate from EFL style POV.

Had to commit style violation for code readability - put each function parameter to a new line.

[WebKit Review Bot]

Attachment 167728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_context.h:241:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:292:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:294:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:295:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:296:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:297:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

(In reply to comment #19)
> Attachment 167728 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/UIProcess/API/efl/ewk_context.h:241:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:292:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:294:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:295:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:296:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:297:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 7 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

as I mentioned in comment #18

[Kenneth Rohde Christiansen]

Comment on attachment 167728 [details]
patch v4

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:297
>> +                                       void* data)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Ah this is the implementation. What is the problem with it being on one line? The idea is that the IDE will wrap it for you.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:74
> + * @brief Type definition for a function that will be called back when @a view did navigation.

did navigation? navigated from one page to another?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:229
> + * The given @a client pointer is not stored anywhere and is used only once for copying of  @c Ewk_Context_History_Client fields.

This comment doesnt seem right

[Mikhail Pozdnyakov]

Created attachment 167740 [details]
patch v5

Fixed issues in documentation, put ewk_context_history_callbacks_set parameters into one line in .cpp file.

[WebKit Review Bot]

Attachment 167740 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_context.h:242:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 167740 [details]
patch v5

Someone else might want to have a final look

[Mikhail Pozdnyakov]

(In reply to comment #24)
> (From update of attachment 167740 [details])
> Someone else might want to have a final look

So, any volunteers?

[Jinwoo Song]

Comment on attachment 167740 [details]
patch v5

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

> Source/WebKit2/UIProcess/API/efl/ewk_navigation_data.h:91
> + * @see ewk_back_forward_list_item_original_uri_get()

nit: uri -> url

[Gyuyoung Kim]

Comment on attachment 167740 [details]
patch v5

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_history_callbacks.cpp:78
> +    // FIXME : WebFrameLoaderClient sends empty title.

nit : FIXME : -> FIXME:

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_history_callbacks.cpp:180
> +    loadUrlSync(httpServer()->getURLForPath(toBeRedirectedPath).data());

Bug 97920 is changing the return type of *loadUrlSync()*. If the patch is landed, you may need to use ASSERT_TRUE for loadUrlSync().

[Mikhail Pozdnyakov]

Created attachment 167947 [details]
to be landed

Addressed comments from Jinwoo and Gyuyoung. Thanks!

[WebKit Review Bot]

Attachment 167947 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_context.h:242:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 167947 [details]
to be landed

Clearing flags on attachment: 167947

Committed r130871: <http://trac.webkit.org/changeset/130871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.