m_isValid in WorkQueue is set as "false" when calling WorkQueue::invalidate(). And the value should be checked before calling the callback function in queue. In the implementation for other port, the checking routine was already added. But, EFL port might be caused a crash because there is no the routine.
Created attachment 175563 [details] Patch
Comment on attachment 175563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175563&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > + MutexLocker locker(m_isValidMutex); > + if (!m_isValid) > + break; The locking scope should be specified. If WorkQueue::invalidate() is called while workItemQueue[i]();, there will be lockup. { MutexLocker locker(m_isValidMutex); if (!m_isValid) break; }
> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:64 > void WorkQueue::platformInvalidate() > { How about adding code to clear m_workItemQueue and m_timerWorkItems in the platformInvalidate()? This also can prevent redundant looping in performXXX function. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:163 > // If a timer work item expired, dispatch the function of the work item. > timerWorkItems[i]->dispatch(); performTimerWork() function have same issue. How about adding the same check code here?
Comment on attachment 175563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175563&action=review >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 >> + break; > > The locking scope should be specified. If WorkQueue::invalidate() is called while workItemQueue[i]();, there will be lockup. > > { > MutexLocker locker(m_isValidMutex); > if (!m_isValid) > break; > } I agree with Byungwoo. Also, wouldn't it make sense to return instead of break is m_isValid is false?
(In reply to comment #2) > (From update of attachment 175563 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175563&action=review > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > + MutexLocker locker(m_isValidMutex); > > + if (!m_isValid) > > + break; > > The locking scope should be specified. If WorkQueue::invalidate() is called while workItemQueue[i]();, there will be lockup. > > { > MutexLocker locker(m_isValidMutex); > if (!m_isValid) > break; > } You're right. I'll fix it/
(In reply to comment #3) > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:64 > > void WorkQueue::platformInvalidate() > > { > How about adding code to clear m_workItemQueue and m_timerWorkItems in the platformInvalidate()? > This also can prevent redundant looping in performXXX function. Actually, I'm not sure the work of the function. How about making new patch for this function? I hope that my patch is just for "m_isValid". > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:163 > > // If a timer work item expired, dispatch the function of the work item. > > timerWorkItems[i]->dispatch(); > performTimerWork() function have same issue. How about adding the same check code here? It's good. It is my missing point. Thanks for your review.
(In reply to comment #6) > (In reply to comment #3) > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:64 > > > void WorkQueue::platformInvalidate() > > > { > > How about adding code to clear m_workItemQueue and m_timerWorkItems in the platformInvalidate()? > > This also can prevent redundant looping in performXXX function. > > Actually, I'm not sure the work of the function. > How about making new patch for this function? > I hope that my patch is just for "m_isValid". Ok, make sens. :)
(In reply to comment #4) > (From update of attachment 175563 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175563&action=review > > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > >> + break; > > > > The locking scope should be specified. If WorkQueue::invalidate() is called while workItemQueue[i]();, there will be lockup. > > > > { > > MutexLocker locker(m_isValidMutex); > > if (!m_isValid) > > break; > > } > > I agree with Byungwoo. Also, wouldn't it make sense to return instead of break is m_isValid is false? Yes, good point. I also think that 'return' is better.
Created attachment 175617 [details] Patch
Created attachment 175619 [details] Patch
(In reply to comment #8) > (In reply to comment #4) > > (From update of attachment 175563 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175563&action=review > > > > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > >> + break; > > > > > > The locking scope should be specified. If WorkQueue::invalidate() is called while workItemQueue[i]();, there will be lockup. > > > > > > { > > > MutexLocker locker(m_isValidMutex); > > > if (!m_isValid) > > > break; > > > } > > > > I agree with Byungwoo. Also, wouldn't it make sense to return instead of break is m_isValid is false? > Yes, good point. I also think that 'return' is better. Ok, I fixed that.
Comment on attachment 175619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175619&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82 > + for (size_t i = 0; i < workItemQueue.size(); ++i) { calculating size each iteration? > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > + Function<void()> function = workItemQueue[i]; Sorry if I missed something, but is it mandatory to have 'function' variable here? Could it be just 'workItemQueue[i]()' after m_isValid check?
(In reply to comment #12) > (From update of attachment 175619 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175619&action=review > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82 > > + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > calculating size each iteration? > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > > + Function<void()> function = workItemQueue[i]; > > Sorry if I missed something, but is it mandatory to have 'function' variable here? Could it be just 'workItemQueue[i]()' after m_isValid check? It is to meet the script "check-webkit-style". I got the below result from the script when removing this line. Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 175619 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175619&action=review > > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82 > > > + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > > > calculating size each iteration? > > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > > > + Function<void()> function = workItemQueue[i]; > > > > Sorry if I missed something, but is it mandatory to have 'function' variable here? Could it be just 'workItemQueue[i]()' after m_isValid check? > > It is to meet the script "check-webkit-style". > I got the below result from the script when removing this line. > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] That would be a bug in the style script then? because the line does NOT contain only semicolon. The style script is there to help, don't change a perfectly good code to work around a style script bug. Mikhail is right that we try to avoid useless temporary variables in WebKit.
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (From update of attachment 175619 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=175619&action=review > > > > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82 > > > > + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > > > > > calculating size each iteration? > > > > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > > > > + Function<void()> function = workItemQueue[i]; > > > > > > Sorry if I missed something, but is it mandatory to have 'function' variable here? Could it be just 'workItemQueue[i]()' after m_isValid check? > > > > It is to meet the script "check-webkit-style". > > I got the below result from the script when removing this line. > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] > > That would be a bug in the style script then? because the line does NOT contain only semicolon. The style script is there to help, don't change a perfectly good code to work around a style script bug. > Mikhail is right that we try to avoid useless temporary variables in WebKit. Ok, it is not important issue to me. I'll fix that.
Created attachment 175643 [details] Patch
Attachment 175643 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 175643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175643&action=review >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 >> + { > > This { should be at the end of the previous line [whitespace/braces] [4] I wish we had 'bool WorkQueue::isValid() const' encapsulating all of this
(In reply to comment #18) > (From update of attachment 175643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175643&action=review > > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > >> + { > > > > This { should be at the end of the previous line [whitespace/braces] [4] > > I wish we had 'bool WorkQueue::isValid() const' encapsulating all of this I think that it is better to use just "function" variable just to avoid the fail for check-webkit-style. WorkQueue::isValid() is redundant if it is required just to avoid the fail for check-webkit-style.
(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 175643 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175643&action=review > > > > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83 > > >> + { > > > > > > This { should be at the end of the previous line [whitespace/braces] [4] > > > > I wish we had 'bool WorkQueue::isValid() const' encapsulating all of this > > I think that it is better to use just "function" variable just to avoid the fail for check-webkit-style. > WorkQueue::isValid() is redundant if it is required just to avoid the fail for check-webkit-style. frankly I don't care about check-webkit-style at all, I just see the same code is used(copied) in several places and I have feeling that a new function would look better and it would not require creating of extra scope.
Created attachment 175761 [details] Patch
Comment on attachment 175761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > + for (size_t i = 0; i < workItemQueue.size(); ++i) { Please cache workItemQueue.size() before the loop. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:86 > + Function<void()> function = workItemQueue[i]; Why this useless variable? > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:93 > + function(); workItemQueue[i](); > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:176 > + MutexLocker locker(m_isValidMutex); Scope? I don't think we wan to keep the mutex locked for timerWorkItems[i]->dispatch() call > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 > - workQueue->performTimerWork(); Why is this removed?
Created attachment 175766 [details] Patch
Comment on attachment 175766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175766&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > + for (size_t i = 0; i < workItemQueue.size(); ++i) { Please cache workItemQueue.size() before the loop > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:86 > + Function<void()> function = workItemQueue[i]; Useless variable. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:93 > + function(); workItemQueue[i](); ? > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 > - workQueue->performTimerWork(); Why is this removed?
Comment on attachment 175761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 >> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > Please cache workItemQueue.size() before the loop. Could you explain why it is required? >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:86 >> + Function<void()> function = workItemQueue[i]; > > Why this useless variable? I hope that this code meets check-webkit-style. >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:176 >> + MutexLocker locker(m_isValidMutex); > > Scope? I don't think we wan to keep the mutex locked for timerWorkItems[i]->dispatch() call Ok, I fixed that. >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 >> - workQueue->performTimerWork(); > > Why is this removed? This function need not be called when m_isValid is false. So, I inserted this function into performWork().
Comment on attachment 175761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review >>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 >>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { >> >> Please cache workItemQueue.size() before the loop. > > Could you explain why it is required? This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. >>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 >>> - workQueue->performTimerWork(); >> >> Why is this removed? > > This function need not be called when m_isValid is false. > So, I inserted this function into performWork(). I see now. I did not see that you moved it. However, moving it has side effects. With your change, performTimerWork() will not be called if m_workItemQueue is empty. Please make sure this is really what we want.
(In reply to comment #26) > (From update of attachment 175761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > >>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > >>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > >> > >> Please cache workItemQueue.size() before the loop. > > > > Could you explain why it is required? > > This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. Is it WebKit coding style? There are so many uses like the style in WebKit. > > >>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 > >>> - workQueue->performTimerWork(); > >> > >> Why is this removed? > > > > This function need not be called when m_isValid is false. > > So, I inserted this function into performWork(). > > I see now. I did not see that you moved it. However, moving it has side effects. > With your change, performTimerWork() will not be called if m_workItemQueue is empty. Please make sure this is really what we want. Thanks for your comment. But I cannot assume the issue. WorkQueue::platformInvalidate() is called when trying to delete WorkQueue. At that time, the work for the items in queue is not required any more. Even WorkQueue::platformInvalidate() in WorkQueueMac.cpp deletes m_dispatchQueue which has the dispatched items.
Comment on attachment 175761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { >>>> >>>> Please cache workItemQueue.size() before the loop. >>> >>> Could you explain why it is required? >> >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > Is it WebKit coding style? There are so many uses like the style in WebKit. From http://www.webkit.org/coding/coding-style.html: size_t frameViewsCount = frameViews.size(); for (size_t i = i; i < frameViewsCount; ++i) frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 >>>>> - workQueue->performTimerWork(); >>>> >>>> Why is this removed? >>> >>> This function need not be called when m_isValid is false. >>> So, I inserted this function into performWork(). >> >> I see now. I did not see that you moved it. However, moving it has side effects. >> With your change, performTimerWork() will not be called if m_workItemQueue is empty. Please make sure this is really what we want. > > Thanks for your comment. But I cannot assume the issue. > WorkQueue::platformInvalidate() is called when trying to delete WorkQueue. At that time, the work for the items in queue is not required any more. > Even WorkQueue::platformInvalidate() in WorkQueueMac.cpp deletes m_dispatchQueue which has the dispatched items. Ok, I try to explain again. Imagine this case: - platformInvalidate() has NOT been called - m_workItemQueue is empty but m_timerWorkItems is not Because you moved performTimerWork() to the end of performWork(), performTimerWork() will never get called (because m_workItemQueue.isEmpty() will be true and performWork() will return early. Therefore, in this scenario, the items in m_workItemQueue will never get processed. This is why I was stating that your change has side effects for cases other than m_isValid is false.
(In reply to comment #28) > (From update of attachment 175761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > >>>> > >>>> Please cache workItemQueue.size() before the loop. > >>> > >>> Could you explain why it is required? > >> > >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > > > Is it WebKit coding style? There are so many uses like the style in WebKit. > > From http://www.webkit.org/coding/coding-style.html: > size_t frameViewsCount = frameViews.size(); > for (size_t i = i; i < frameViewsCount; ++i) > frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); > Could check the title for your example? "Prefer index over iterator in Vector iterations for a terse, easier-to-read code." As I think, it means that index style "[i]" is recommended instead of iterator style "::iterator", ".begin()". > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:-179 > >>>>> - workQueue->performTimerWork(); > >>>> > >>>> Why is this removed? > >>> > >>> This function need not be called when m_isValid is false. > >>> So, I inserted this function into performWork(). > >> > >> I see now. I did not see that you moved it. However, moving it has side effects. > >> With your change, performTimerWork() will not be called if m_workItemQueue is empty. Please make sure this is really what we want. > > > > Thanks for your comment. But I cannot assume the issue. > > WorkQueue::platformInvalidate() is called when trying to delete WorkQueue. At that time, the work for the items in queue is not required any more. > > Even WorkQueue::platformInvalidate() in WorkQueueMac.cpp deletes m_dispatchQueue which has the dispatched items. > > Ok, I try to explain again. Imagine this case: > - platformInvalidate() has NOT been called > - m_workItemQueue is empty but m_timerWorkItems is not > > Because you moved performTimerWork() to the end of performWork(), performTimerWork() will never get called (because m_workItemQueue.isEmpty() will be true and performWork() will return early. Therefore, in this scenario, the items in m_workItemQueue will never get processed. This is why I was stating that your change has side effects for cases other than m_isValid is false. Thanks for the detail. I missed "return" for "m_workItemQueue.isEmpty()". I fixed that.
(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 175761 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > > > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > >>>> > > >>>> Please cache workItemQueue.size() before the loop. > > >>> > > >>> Could you explain why it is required? > > >> > > >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > > > > > Is it WebKit coding style? There are so many uses like the style in WebKit. > > > > From http://www.webkit.org/coding/coding-style.html: > > size_t frameViewsCount = frameViews.size(); > > for (size_t i = i; i < frameViewsCount; ++i) > > frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); > > > > Could check the title for your example? > "Prefer index over iterator in Vector iterations for a terse, easier-to-read code." > As I think, it means that index style "[i]" is recommended instead of iterator style "::iterator", ".begin()". Yes, I know the section title is about a different matter. It still show that code in the style guide (which is supposed to be good reference) is caching the container size before the loop. This is common practice in WebKit code base (at least for new patches). This is good practice too. I'm not going to discuss this further since it is a style issue. I really hope the patch lands with the right style though (confirm with Gyuyoung if needed).
And by the way, Mikhail made the exact same comment: https://bugs.webkit.org/show_bug.cgi?id=103006#c12 so I'm not the only one.
(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 175761 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > > > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > >>>> > > >>>> Please cache workItemQueue.size() before the loop. > > >>> > > >>> Could you explain why it is required? > > >> > > >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > > > > > Is it WebKit coding style? There are so many uses like the style in WebKit. > > > > From http://www.webkit.org/coding/coding-style.html: > > size_t frameViewsCount = frameViews.size(); > > for (size_t i = i; i < frameViewsCount; ++i) > > frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); > > > > Could check the title for your example? > "Prefer index over iterator in Vector iterations for a terse, easier-to-read code." > As I think, it means that index style "[i]" is recommended instead of iterator style "::iterator", ".begin()". > We are not lawyers here. What's the problem to cache container size? it makes code better anyway.
(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > (From update of attachment 175761 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > > > > > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > > >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > > >>>> > > > >>>> Please cache workItemQueue.size() before the loop. > > > >>> > > > >>> Could you explain why it is required? > > > >> > > > >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > > > > > > > Is it WebKit coding style? There are so many uses like the style in WebKit. > > > > > > From http://www.webkit.org/coding/coding-style.html: > > > size_t frameViewsCount = frameViews.size(); > > > for (size_t i = i; i < frameViewsCount; ++i) > > > frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); > > > > > > > Could check the title for your example? > > "Prefer index over iterator in Vector iterations for a terse, easier-to-read code." > > As I think, it means that index style "[i]" is recommended instead of iterator style "::iterator", ".begin()". > > Yes, I know the section title is about a different matter. It still show that code in the style guide (which is supposed to be good reference) is caching the container size before the loop. This is common practice in WebKit code base (at least for new patches). This is good practice too. > > I'm not going to discuss this further since it is a style issue. I really hope the patch lands with the right style though (confirm with Gyuyoung if needed). Thanks for your review. I appreciate it. size() in Vector returns just the member variable, not computed. "size_t size() const { return m_size; }" in Vector.h So, it's unnecessary like your comment "we try to avoid useless temporary variables in WebKit."
(In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #29) > > > (In reply to comment #28) > > > > (From update of attachment 175761 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=175761&action=review > > > > > > > > >>>>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > > > > >>>>> + for (size_t i = 0; i < workItemQueue.size(); ++i) { > > > > >>>> > > > > >>>> Please cache workItemQueue.size() before the loop. > > > > >>> > > > > >>> Could you explain why it is required? > > > > >> > > > > >> This is WebKit coding style and this makes sure that the queue size is not retrieved (possibly computed) for every iteration of the loop. > > > > > > > > > > Is it WebKit coding style? There are so many uses like the style in WebKit. > > > > > > > > From http://www.webkit.org/coding/coding-style.html: > > > > size_t frameViewsCount = frameViews.size(); > > > > for (size_t i = i; i < frameViewsCount; ++i) > > > > frameViews[i]->updateLayoutAndStyleIfNeededRecursive(); > > > > > > > > > > Could check the title for your example? > > > "Prefer index over iterator in Vector iterations for a terse, easier-to-read code." > > > As I think, it means that index style "[i]" is recommended instead of iterator style "::iterator", ".begin()". > > > > Yes, I know the section title is about a different matter. It still show that code in the style guide (which is supposed to be good reference) is caching the container size before the loop. This is common practice in WebKit code base (at least for new patches). This is good practice too. > > > > I'm not going to discuss this further since it is a style issue. I really hope the patch lands with the right style though (confirm with Gyuyoung if needed). > > Thanks for your review. I appreciate it. > size() in Vector returns just the member variable, not computed. > "size_t size() const { return m_size; }" in Vector.h > So, it's unnecessary like your comment "we try to avoid useless temporary variables in WebKit." Sure, we know that in practice and with the current implementation it makes no difference. However, it is still good practice as it does not rely on the underlying implementation of the given container. If the container is changed later or its implementation, the code may not perform as well anymore. As I said, it is mostly about style and good practices established in the project. Your argument about other places in the code not caching the container size does not really stand because upstream does not usually accept clean up patches. Therefore, the style rules only apply to new code.
Created attachment 175781 [details] Patch
Comment on attachment 175781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175781&action=review Commented before but I'm reiterating so that my comments appears in the patch being reviewed. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:85 > + for (size_t i = 0; i < workItemQueue.size(); ++i) { Common practice in WebKit is to cache container size before the loop. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:86 > + Function<void()> function = workItemQueue[i]; Useless temporary variable, we try to avoid those in WebKit. In this case, it is even worse because it is constructed before the isValid check. > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:93 > + function(); workItemQueue[i]();
Comment on attachment 175781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175781&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:69 > + waitForThreadCompletion(m_workQueueThread); > + m_workQueueThread = 0; It looks ok. But to clarify that this has no side effect, can you share the test result with this? (like checking regressions of layout test or API test) And if you changed your mind to touch the platformInvalidate() function in this patch, how about your opinion for the suggestion of clearing the workItemQueue and timerWorkItems in this function that I commented before? >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:86 >> + Function<void()> function = workItemQueue[i]; > > Useless temporary variable, we try to avoid those in WebKit. In this case, it is even worse because it is constructed before the isValid check. I also agree about this. Lazy instantiation is the common and preferred usage in C++. And there is a possible way not to make additional variable. And I cannot find the advantage to make this variable. I also think that style check code has some bug. (I cannot understand what is the exact style violation without this line) > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:91 > + { > + MutexLocker locker(m_isValidMutex); > + if (!m_isValid) > + return; > + } How about making a function to check m_isValid as Mikhail commented above? if (!isValid()) return; I think that the solution itself has advantage for the code size and readability. You can make it as inline function if you have doubt on performance for calling function.
Any update on this ?
Comment on attachment 175781 [details] Patch Cleared review? from attachment 175781 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.
Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.