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()
Created attachment 186823 [details] patch
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?
(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 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.
Created attachment 186835 [details] patch v2 Updated change log.
(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..
(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
LGTM
Comment on attachment 186835 [details] patch v2 Attachment 186835 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16397188
(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.
Could please anyone from WK2 owners list review this (sign it off for WK2)?
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 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 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 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.
> >>> 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.
Created attachment 187354 [details] patch v3 took comments from Chris into consideration.
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 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 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.
Committed r143007: <http://trac.webkit.org/changeset/143007>