Bug 109165 – [WK2][EFL] Removal of non coordinated graphics code path from WK2 EFL

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA. Please refer to webkit-efl ML for details: https://lists.webkit.org/pipermail/webkit-efl/2013-February/000497.html

[Mikhail Pozdnyakov]

Created attachment 187044 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 187044 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
> -    inline Evas_GL_Context* context() const { return m_context; }
> +    Evas_GL_Context* context() { return m_context; }

why unrelated changed?

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 187044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187044&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
> > -    inline Evas_GL_Context* context() const { return m_context; }
> > +    Evas_GL_Context* context() { return m_context; }
> 
> why unrelated changed?

I just could not pass through :) but it's off course worth mentioning in change log

[Mikhail Pozdnyakov]

Created attachment 187050 [details]
patch v2

updated changelog.

[Kalyan]

Comment on attachment 187050 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:156
> +    void update(const WebCore::IntRect& rect = WebCore::IntRect());

should the semantics change?? we don't care about the rect and do a full update with coordinategraphics

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 187050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187050&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.h:156
> > +    void update(const WebCore::IntRect& rect = WebCore::IntRect());
> 
> should the semantics change?? we don't care about the rect and do a full update with coordinategraphics

Yeah, thanks for the catch!

[Mikhail Pozdnyakov]

Created attachment 187065 [details]
patch v3

Took Kalyan's comment into consideration.

[Mikhail Pozdnyakov]

Created attachment 187079 [details]
patch v4

Also renamed EwkView::update() to EwkView::scheduleUpdateDisplay.

[Kalyan]

(In reply to comment #8)
> Created an attachment (id=187079) [details]
> patch v4
> 
> Also renamed EwkView::update() to EwkView::scheduleUpdateDisplay.

Should we name it something like scheduleSceneUpdate (to represent what we do in timer callback function).

otherwise LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 187079 [details]
patch v4

LGTM

[Viatcheslav Ostapenko]

Comment on attachment 187079 [details]
patch v4

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

> Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
> +    Evas_GL_Context* context() { return m_context; }

Why do you remove const here? Also inline doesn't make any harm.

> Source/WebKit2/UIProcess/API/efl/EvasGLSurface.h:56
> +    Evas_GL_Surface* surface() { return m_surface; }

ditto

[Chris Dumez]

Comment on attachment 187079 [details]
patch v4

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

>> Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
>> +    Evas_GL_Context* context() { return m_context; }
> 
> Why do you remove const here? Also inline doesn't make any harm.

It was discussed on the webkit-dev mailing list not too long ago that we should not make such methods const unless they return a const pointer. Since pointer being returned is not const, I think removing const is correct. However, I agree that keeping the inline statement does not hurt.

[Viatcheslav Ostapenko]

(In reply to comment #12)
> (From update of attachment 187079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187079&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
> >> +    Evas_GL_Context* context() { return m_context; }
> > 
> > Why do you remove const here? Also inline doesn't make any harm.
> 
> It was discussed on the webkit-dev mailing list not too long ago that we should not make such methods const unless they return a const pointer. Since pointer being returned is not const, I think removing const is correct. However, I agree that keeping the inline statement does not hurt.

As I remember, there was not conclusion. At least I didn't see any updates to webkit style or anywhere else. Honestly, I didn't see any reasonable points in this discussion in any case.
Removing "const" hints the compiler, that object might change inside method and compiler will not optimize usage of this method.
In this case compiler has the method body, so it will inline and optimize it in any case, so I don't like this just because it is unrelated change, but in general blindly removing consts is a bad idea.

[Mikhail Pozdnyakov]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 187079 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187079&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/EvasGLContext.h:48
> > >> +    Evas_GL_Context* context() { return m_context; }
> > > 
> > > Why do you remove const here? Also inline doesn't make any harm.
> > 
> > It was discussed on the webkit-dev mailing list not too long ago that we should not make such methods const unless they return a const pointer. Since pointer being returned is not const, I think removing const is correct. However, I agree that keeping the inline statement does not hurt.
> 
> As I remember, there was not conclusion. At least I didn't see any updates to webkit style or anywhere else. Honestly, I didn't see any reasonable points in this discussion in any case.
> Removing "const" hints the compiler, that object might change inside method and compiler will not optimize usage of this method.
> In this case compiler has the method body, so it will inline and optimize it in any case, so I don't like this just because it is unrelated change, but in general blindly removing consts is a bad idea.

Const methods are not used for compiler optimization but rather used to show logical constness! 

The ML discussion around this: https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html. 

Having const in this particular case makes absolutely no sense.

[Viatcheslav Ostapenko]

> Const methods are not used for compiler optimization but rather used to show logical constness! 

Yes, it is used.


> The ML discussion around this: https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html. 
> 
> Having const in this particular case makes absolutely no sense.

Yes, it is.
For example, if there is several calls to const method, compiler might optimize it to single call and keep it in temporary variable. Compiler also could reorder calls to const methods if necessary.
If the method is not const, it is not allowed because non-const method are assumed to change object state.

> The ML discussion around this: https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html. 

Yes, I know. I've seen it.

[WebKit Review Bot]

Comment on attachment 187079 [details]
patch v4

Clearing flags on attachment: 187079

Committed r142169: <http://trac.webkit.org/changeset/142169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mikhail Pozdnyakov]

(In reply to comment #15)
> > Const methods are not used for compiler optimization but rather used to show logical constness! 
> 
> Yes, it is used.
> 
> 
> > The ML discussion around this: https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html. 
> > 
> > Having const in this particular case makes absolutely no sense.
> 
> Yes, it is.
> For example, if there is several calls to const method, compiler might optimize it to single call and keep it in temporary variable. Compiler also could reorder calls to const methods if necessary.
> If the method is not const, it is not allowed because non-const method are assumed to change object state.
> 
> > The ML discussion around this: https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html. 
> 
> Yes, I know. I've seen it.

The definition is: 
"const member function - member function declared not to modify the state of the object for which it is called." (Bjarne Stroustrup: The C++ Programming Language (Special Edition)). Nothing is written about optimization.

The fact that compiler might make optimizations for const methods does not mean that const methods should be treated as means of optimization. They should be used so that logical constness of the object is kept.