SSIA. Please refer to webkit-efl ML for details: https://lists.webkit.org/pipermail/webkit-efl/2013-February/000497.html
Created attachment 187044 [details] patch
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?
(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
Created attachment 187050 [details] patch v2 updated changelog.
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
(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!
Created attachment 187065 [details] patch v3 Took Kalyan's comment into consideration.
Created attachment 187079 [details] patch v4 Also renamed EwkView::update() to EwkView::scheduleUpdateDisplay.
(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 on attachment 187079 [details] patch v4 LGTM
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 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.
(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.
(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.
> 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 on attachment 187079 [details] patch v4 Clearing flags on attachment: 187079 Committed r142169: <http://trac.webkit.org/changeset/142169>
All reviewed patches have been landed. Closing bug.
(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.