Bug 109038 - [WK2][EFL]REGRESSION (r141978): ewk_view_type_check api test failing
Summary: [WK2][EFL]REGRESSION (r141978): ewk_view_type_check api test failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks: 106752
  Show dependency treegraph
 
Reported: 2013-02-06 03:29 PST by Mikhail Pozdnyakov
Modified: 2013-02-15 09:09 PST (History)
11 users (show)

See Also:


Attachments
patch (4.22 KB, patch)
2013-02-06 04:28 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (4.37 KB, patch)
2013-02-06 05:24 PST, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v3 (4.29 KB, patch)
2013-02-08 13:08 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v4 (4.70 KB, patch)
2013-02-08 14:02 PST, Mikhail Pozdnyakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-02-06 03:29:09 PST
SSIA. Assertion.
ASSERTION FAILED: evasObject && isViewEvasObject(evasObject)
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/EwkView.cpp(1393) : EwkView* toEwkView(const Evas_Object*)
1   0x2aac5162c073 toEwkView(_Evas_Object const*)
2   0x2aac5164b8c1 ewk_view_context_get
3   0x448cf3 EWK2UnitTestBase_ewk_view_type_check_Test::TestBody()
4   0x2aac52a49fdd testing::Test::Run()
5   0x2aac52a4a601 testing::internal::TestInfoImpl::Run()
6   0x2aac52a4ab72 testing::TestCase::Run()
7   0x2aac52a4f53e testing::internal::UnitTestImpl::RunAllTests()
8   0x2aac52a4e2bc testing::UnitTest::Run()
Comment 1 Mikhail Pozdnyakov 2013-02-06 04:28:05 PST
Created attachment 186823 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-02-06 05:14:05 PST
Comment on attachment 186823 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> +        if (Ewk_View_Smart_Data* smartData = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(evasObject)))
> +            return smartData->priv;

Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar

> Source/WebKit2/UIProcess/API/efl/EwkView.h:-319
>  EwkView* toEwkView(const Evas_Object*);
> -EwkView* toEwkView(const Ewk_View_Smart_Data* smartData);

You removed the smartData version. That deserves a comment in the changelog. It is only used internally?
Comment 3 Mikhail Pozdnyakov 2013-02-06 05:19:17 PST
(In reply to comment #2)
> (From update of attachment 186823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> > +        if (Ewk_View_Smart_Data* smartData = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(evasObject)))
> > +            return smartData->priv;
> 
> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar

well, evas_object_smart_data_get itself can return null:
"A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."

> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.h:-319
> >  EwkView* toEwkView(const Evas_Object*);
> > -EwkView* toEwkView(const Ewk_View_Smart_Data* smartData);
> 
> You removed the smartData version. That deserves a comment in the changelog. It is only used internally?

yes, it's used only within EwkView. I'll add the comment.
Comment 4 Kenneth Rohde Christiansen 2013-02-06 05:20:20 PST
Comment on attachment 186823 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
>>> +            return smartData->priv;
>> 
>> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> 
> well, evas_object_smart_data_get itself can return null:
> "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."

But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.
Comment 5 Mikhail Pozdnyakov 2013-02-06 05:24:19 PST
Created attachment 186835 [details]
patch v2

Updated change log.
Comment 6 Mikhail Pozdnyakov 2013-02-06 05:29:35 PST
(In reply to comment #4)
> (From update of attachment 186823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> >>> +            return smartData->priv;
> >> 
> >> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> > 
> > well, evas_object_smart_data_get itself can return null:
> > "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."
> 
> But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.

indeed, re-uploading..
Comment 7 Mikhail Pozdnyakov 2013-02-06 05:44:29 PST
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 186823 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> > >>> +            return smartData->priv;
> > >> 
> > >> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> > > 
> > > well, evas_object_smart_data_get itself can return null:
> > > "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."
> > 
> > But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.
> 
> indeed, re-uploading..

well, not actually as isViewEvasObject() checks that evas object belongs to a certain Smart class, but does not make sure that the given evas object has smart data set. Putting r? again
Comment 8 Kenneth Rohde Christiansen 2013-02-06 05:45:32 PST
LGTM
Comment 9 Build Bot 2013-02-06 07:17:49 PST
Comment on attachment 186835 [details]
patch v2

Attachment 186835 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16397188
Comment 10 Mikhail Pozdnyakov 2013-02-06 07:23:12 PST
(In reply to comment #9)
> (From update of attachment 186835 [details])
> Attachment 186835 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16397188

Some problems on win-ews. Unrelated to the patch.
Comment 11 Mikhail Pozdnyakov 2013-02-08 11:34:32 PST
Could please anyone from WK2 owners list review this (sign it off for WK2)?
Comment 12 Chris Dumez 2013-02-08 11:38:12 PST
Comment on attachment 186835 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)

Why did this function get moved?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
> +    if (evasObject && isViewEvasObject(evasObject)) {

Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.
Comment 13 Chris Dumez 2013-02-08 11:42:41 PST
Comment on attachment 186835 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
> +EwkView* toEwkViewChecked(const Evas_Object*);

If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?
Comment 14 Mikhail Pozdnyakov 2013-02-08 11:57:55 PST
Comment on attachment 186835 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
>> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)
> 
> Why did this function get moved?

because it's not used outside, no need to export.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
>> +    if (evasObject && isViewEvasObject(evasObject)) {
> 
> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.

to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..

>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
>> +EwkView* toEwkViewChecked(const Evas_Object*);
> 
> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?

Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.
Comment 15 Chris Dumez 2013-02-08 12:07:26 PST
Comment on attachment 186835 [details]
patch v2

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

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
>>> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)
>> 
>> Why did this function get moved?
> 
> because it's not used outside, no need to export.

I see, makes sense.

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
>>> +    if (evasObject && isViewEvasObject(evasObject)) {
>> 
>> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.
> 
> to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
> BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..

This is called only in our public EFL API implementation. We want to emit warnings on bad arguments. Sure, EWK_VIEW_IMPL_GET_OR_RETURN will print a warning but we won't know which check has failed: Was the argument NULL? Was the argument not a view object? Was the smart data missing?

>>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
>>> +EwkView* toEwkViewChecked(const Evas_Object*);
>> 
>> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?
> 
> Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.

This function is used only in ewk_view.cpp and is used only for checking arguments to our EFL C API. I still think it should be in ewk_view.cpp instead of EwkView (which is private implementation). We do not want people to start using this function is EwkView.cpp as it does not assert.
Comment 16 Mikhail Pozdnyakov 2013-02-08 13:07:29 PST
 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
> >>> +    if (evasObject && isViewEvasObject(evasObject)) {
> >> 
> >> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.
> > 
> > to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
> > BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..
> 
> This is called only in our public EFL API implementation. We want to emit warnings on bad arguments. Sure, EWK_VIEW_IMPL_GET_OR_RETURN will print a warning but we won't know which check has failed: Was the argument NULL? Was the argument not a view object? Was the smart data missing?
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
> >>> +EwkView* toEwkViewChecked(const Evas_Object*);
> >> 
> >> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?
> > 
> > Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.
> 
> This function is used only in ewk_view.cpp and is used only for checking arguments to our EFL C API. I still think it should be in ewk_view.cpp instead of EwkView (which is private implementation). We do not want people to start using this function is EwkView.cpp as it does not assert.

ok, let's do so.
Comment 17 Mikhail Pozdnyakov 2013-02-08 13:08:59 PST
Created attachment 187354 [details]
patch v3

took comments from Chris into consideration.
Comment 18 Mikhail Pozdnyakov 2013-02-08 14:02:30 PST
Created attachment 187362 [details]
patch v4

1) isViewEvasObject can emit warnings itself, so no need to put it inside EINA safety check.
2) renamed isViewEvasObject to isEwkViewEvasObject to be consistent with other function names.
Comment 19 Chris Dumez 2013-02-08 14:14:09 PST
Comment on attachment 187362 [details]
patch v4

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

LGTM. Thanks.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:124
> +    ASSERT(smartData && smartData->priv);

nit: Wouldn't it be better with 2 assertions so that we know which one fails?
Comment 20 Anders Carlsson 2013-02-15 08:40:44 PST
Comment on attachment 187362 [details]
patch v4

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1365
> -    ASSERT(evasObject && isViewEvasObject(evasObject));
> +    ASSERT(evasObject && isEwkViewEvasObject(evasObject));

Same comment about the assert here.
Comment 21 Mikhail Pozdnyakov 2013-02-15 09:09:46 PST
Committed r143007: <http://trac.webkit.org/changeset/143007>