Bug 109165

Summary: [WK2][EFL] Removal of non coordinated graphics code path from WK2 EFL
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kalyan.kondapally, kenneth, lucas.de.marchi, ostap73, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch v2
none
patch v3
none
patch v4 none

Description Mikhail Pozdnyakov 2013-02-07 03:13:07 PST
SSIA. Please refer to webkit-efl ML for details: https://lists.webkit.org/pipermail/webkit-efl/2013-February/000497.html
Comment 1 Mikhail Pozdnyakov 2013-02-07 03:28:00 PST
Created attachment 187044 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-02-07 03:30:45 PST
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?
Comment 3 Mikhail Pozdnyakov 2013-02-07 03:53:32 PST
(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
Comment 4 Mikhail Pozdnyakov 2013-02-07 04:00:07 PST
Created attachment 187050 [details]
patch v2

updated changelog.
Comment 5 Kalyan 2013-02-07 04:47:22 PST
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
Comment 6 Mikhail Pozdnyakov 2013-02-07 05:01:07 PST
(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!
Comment 7 Mikhail Pozdnyakov 2013-02-07 05:02:50 PST
Created attachment 187065 [details]
patch v3

Took Kalyan's comment into consideration.
Comment 8 Mikhail Pozdnyakov 2013-02-07 05:53:19 PST
Created attachment 187079 [details]
patch v4

Also renamed EwkView::update() to EwkView::scheduleUpdateDisplay.
Comment 9 Kalyan 2013-02-07 07:35:35 PST
(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.
Comment 10 Kenneth Rohde Christiansen 2013-02-07 08:28:02 PST
Comment on attachment 187079 [details]
patch v4

LGTM
Comment 11 Viatcheslav Ostapenko 2013-02-07 09:28:45 PST
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
Comment 12 Chris Dumez 2013-02-07 09:38:13 PST
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.
Comment 13 Viatcheslav Ostapenko 2013-02-07 10:47:53 PST
(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.
Comment 14 Mikhail Pozdnyakov 2013-02-07 11:57:25 PST
(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.
Comment 15 Viatcheslav Ostapenko 2013-02-07 12:15:21 PST
> 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.
Comment 16 WebKit Review Bot 2013-02-07 13:21:50 PST
Comment on attachment 187079 [details]
patch v4

Clearing flags on attachment: 187079

Committed r142169: <http://trac.webkit.org/changeset/142169>
Comment 17 WebKit Review Bot 2013-02-07 13:21:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Mikhail Pozdnyakov 2013-02-07 15:23:21 PST
(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.