[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 #34 from Christophe Dumez <christophe.dumez at intel.com>  2012-11-23 04:33:21 PST ---
(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.

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