[Webkit-unassigned] [Bug 103006] [EFL][WK2] Missing the routine to check the validation for workqueue

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103006





--- Comment #29 from Jongseok Yang <js45.yang at samsung.com>  2012-11-23 03:58:24 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.