[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 #37 from Byungwoo Lee <bw80.lee at samsung.com>  2012-11-25 17:43:35 PST ---
(From update of attachment 175781)
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.

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