isEwkViewEvasObject is general check routine of EFL and mainly used in ewk_view.cpp but it was in EwkView.cpp since separated EwkView.cpp from ewk_view.cpp
Created attachment 221610 [details] Patch
Comment on attachment 221610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > Source/WebKit2/ChangeLog:9 > + ewk_view.cpp but it was in EwkView.cpp since separated EwkView.cpp from ewk_view.cpp Isn't any possibility used by other ewk files ?
(In reply to comment #2) > (From update of attachment 221610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > > > Source/WebKit2/ChangeLog:9 > > + ewk_view.cpp but it was in EwkView.cpp since separated EwkView.cpp from ewk_view.cpp > > Isn't any possibility used by other ewk files ? Sure, I believe that internal files must use the object correctly and they should not call it directly (except EwkView.cpp). The major purpose of this function is for public APIs which we can not guarantee the argument.
Comment on attachment 221610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review >>> Source/WebKit2/ChangeLog:9 >>> + ewk_view.cpp but it was in EwkView.cpp since separated EwkView.cpp from ewk_view.cpp >> >> Isn't any possibility used by other ewk files ? > > Sure, I believe that internal files must use the object correctly and they should not call it directly (except EwkView.cpp). > > The major purpose of this function is for public APIs which we can not guarantee the argument. I don't have a big opinion to move it to ewk_view.cpp. However, I don't see what is benefit when we move it from EwkView.cpp to ewk_view.cpp. Rather, we need to add additional a member function as below, static const char smartClassName[]; Is there any benefit ?
(In reply to comment #4) > (From update of attachment 221610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > > >>> Source/WebKit2/ChangeLog:9 > >>> + ewk_view.cpp but it was in EwkView.cpp since separated EwkView.cpp from ewk_view.cpp > >> > >> Isn't any possibility used by other ewk files ? > > > > Sure, I believe that internal files must use the object correctly and they should not call it directly (except EwkView.cpp). > > > > The major purpose of this function is for public APIs which we can not guarantee the argument. > > I don't have a big opinion to move it to ewk_view.cpp. However, I don't see what is benefit when we move it from EwkView.cpp to ewk_view.cpp. > > Rather, we need to add additional a member function as below, > static const char smartClassName[]; > > Is there any benefit ? Now, every API should call it to check validity. After moved, it will be inlined. Although it is not big advantage, I think that it should be better and logically right. smartClassName is exposed but it's only static and it is already defined in code. So, I think that it's fine.
Comment on attachment 221610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-126 > - ASSERT(isEwkViewEvasObject(evasObject)); Are you sure we don't need to check this one here ?
(In reply to comment #6) > (From update of attachment 221610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-126 > > - ASSERT(isEwkViewEvasObject(evasObject)); > > Are you sure we don't need to check this one here ? I briefly checked and I think that its call path is too simple to doubt the type of evasObject. Callers only called using m_evasObject in EwkView.cpp locally. Anyway, We may want to use isEwkViewEvasObject for assertion in EwkView.cpp (There are only few cases like above) while we must use it for verification in ewk_view.cpp. If we should consider the assertions, I can move it to EwkView.h.
Comment on attachment 221610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-126 >>> - ASSERT(isEwkViewEvasObject(evasObject)); >> >> Are you sure we don't need to check this one here ? > > I briefly checked and I think that its call path is too simple to doubt the type of evasObject. > > Callers only called using m_evasObject in EwkView.cpp locally. > > Anyway, > We may want to use isEwkViewEvasObject for assertion in EwkView.cpp (There are only few cases like above) while we must use it for verification in ewk_view.cpp. > > If we should consider the assertions, I can move it to EwkView.h. I think it is safer to keep this assertion on debug. Please keep it.
Created attachment 221714 [details] Patch
(In reply to comment #8) > (From update of attachment 221610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221610&action=review > > >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-126 > >>> - ASSERT(isEwkViewEvasObject(evasObject)); > >> > >> Are you sure we don't need to check this one here ? > > > > I briefly checked and I think that its call path is too simple to doubt the type of evasObject. > > > > Callers only called using m_evasObject in EwkView.cpp locally. > > > > Anyway, > > We may want to use isEwkViewEvasObject for assertion in EwkView.cpp (There are only few cases like above) while we must use it for verification in ewk_view.cpp. > > > > If we should consider the assertions, I can move it to EwkView.h. > > I think it is safer to keep this assertion on debug. Please keep it. OK, I moved.
Comment on attachment 221714 [details] Patch Though I don't this this patch has a big benefit, it looks there is no critical problem. rs=me.
(In reply to comment #11) > (From update of attachment 221714 [details]) > Though I don't this this patch has a big benefit, it looks there is no critical problem. rs=me. s/don't this/don't think/g
Comment on attachment 221714 [details] Patch Clearing flags on attachment: 221714 Committed r162426: <http://trac.webkit.org/changeset/162426>
All reviewed patches have been landed. Closing bug.