Bug 127261 – [EFL][WK2] Move isEwkViewEvasObject back to ewk_view.cpp

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

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 221610 [details]
Patch

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

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

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

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

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 221714 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

(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

[WebKit Commit Bot]

Comment on attachment 221714 [details]
Patch

Clearing flags on attachment: 221714

Committed r162426: <http://trac.webkit.org/changeset/162426>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.