Bug 93983

Summary: [BlackBerry] Robust-fy the LayerWebKitThread ownership with InRegionScroller
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: WebKit BlackBerryAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: mifenton, tonikitoo, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch - style issue fixed yong.li.webkit: review+

Description Antonio Gomes 2012-08-14 09:39:25 PDT
That ensure we have a thread safe cache of it

It came from a offline discussion with Arvid.
Comment 1 Antonio Gomes 2012-08-14 14:07:41 PDT
Created attachment 158412 [details]
patch
Comment 2 WebKit Review Bot 2012-08-14 14:09:25 PDT
Attachment 158412 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/InRegionScrol..." exit_code: 1
Source/WebKit/blackberry/Api/InRegionScroller.cpp:97:  Missing space before ( in while(  [whitespace/parens] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antonio Gomes 2012-08-14 14:13:06 PDT
Created attachment 158413 [details]
patch - style issue fixed
Comment 4 Yong Li 2012-08-14 14:21:51 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=158412&action=review

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:100
> +    while(it != end) {
> +        delete *it;
> +        ++it;
> +    }

WebKit usually use this pattern:

for (size_t i = 0; i < vector.size(); ++i)
   delete vector[i];

Another question, why not use WTF::Vector? It has a method deleteAllValues()

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:155
> +    ASSERT(!m_activeInRegionScrollableAreas.size());

empty() is better than !size()

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:236
> +std::vector<Platform::ScrollViewBase*> InRegionScrollerPrivate::activeInRegionScrollableAreas() const
> +{
> +    return m_activeInRegionScrollableAreas;
>  }

Is there any reason to return a copy? Can we use const std::vector& ?

> Source/WebKit/blackberry/Api/InRegionScroller_p.h:73
> +    std::vector<Platform::ScrollViewBase*> m_activeInRegionScrollableAreas;

We should prefer WTF::Vector to std::vector

> Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:45
> +    m_cachedCompositedScrollableLayer = 0;

Why is this needed?
Comment 5 Antonio Gomes 2012-08-14 14:25:11 PDT
(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=158412&action=review
> 
> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:100
> > +    while(it != end) {
> > +        delete *it;
> > +        ++it;
> > +    }
> 
> WebKit usually use this pattern:
> 
> for (size_t i = 0; i < vector.size(); ++i)
>    delete vector[i];
 
Sure. Can change.

> Another question, why not use WTF::Vector? It has a method deleteAllValues()

<anilsson> agomes_: why is using std::vector instead of WTF::Vector?
<anilsson> agomes_: WTF::Vector has convenient delete method that calls delete on each member
<anilsson> oh hang on, it's probably because you'll pass the whole vector to platform at some point?
<agomes_> yes :/

> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:155
> > +    ASSERT(!m_activeInRegionScrollableAreas.size());
> 
> empty() is better than !size()

Sure

> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:236
> > +std::vector<Platform::ScrollViewBase*> InRegionScrollerPrivate::activeInRegionScrollableAreas() const
> > +{
> > +    return m_activeInRegionScrollableAreas;
> >  }
> 
> Is there any reason to return a copy? Can we use const std::vector& ?

It is in a follow-up: Bug 94021  (please review).

I can change the stuff you suggested before landing, if you have no more concerns.


> > Source/WebKit/blackberry/Api/InRegionScroller_p.h:73
> > +    std::vector<Platform::ScrollViewBase*> m_activeInRegionScrollableAreas;
> 
> We should prefer WTF::Vector to std::vector
> 
> > Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:45
> > +    m_cachedCompositedScrollableLayer = 0;
> 
> Why is this needed?
Comment 6 Antonio Gomes 2012-08-14 14:26:41 PDT
> > > Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:45
> > > +    m_cachedCompositedScrollableLayer = 0;
> > 
> > Why is this needed?

Not all layers support composited scrolling.
Comment 7 Antonio Gomes 2012-08-14 14:27:18 PDT
(In reply to comment #6)
> > > > Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:45
> > > > +    m_cachedCompositedScrollableLayer = 0;
> > > 
> > > Why is this needed?
> 
> Not all layers support composited scrolling.

Err, ignore that. It was just to clear the RefPtr. Is it not needed?
Comment 8 Yong Li 2012-08-14 14:41:43 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158412&action=review
> > 
> > > Source/WebKit/blackberry/Api/InRegionScroller.cpp:100
> > > +    while(it != end) {
> > > +        delete *it;
> > > +        ++it;
> > > +    }
> > 
> > WebKit usually use this pattern:
> > 
> > for (size_t i = 0; i < vector.size(); ++i)
> >    delete vector[i];
> 
> Sure. Can change.
> 
> > Another question, why not use WTF::Vector? It has a method deleteAllValues()
> 
> <anilsson> agomes_: why is using std::vector instead of WTF::Vector?
> <anilsson> agomes_: WTF::Vector has convenient delete method that calls delete on each member
> <anilsson> oh hang on, it's probably because you'll pass the whole vector to platform at some point?
> <agomes_> yes :/

Hm... not a big deal.

> > > Source/WebKit/blackberry/Api/InRegionScroller.cpp:236
> > > +std::vector<Platform::ScrollViewBase*> InRegionScrollerPrivate::activeInRegionScrollableAreas() const
> > > +{
> > > +    return m_activeInRegionScrollableAreas;
> > >  }
> > 
> > Is there any reason to return a copy? Can we use const std::vector& ?
> 
> It is in a follow-up: Bug 94021  (please review).
> 
> I can change the stuff you suggested before landing, if you have no more concerns.

Yeah. it should be independent to Bug 94021. could save one copy
Comment 9 Yong Li 2012-08-14 14:42:44 PDT
Comment on attachment 158413 [details]
patch - style issue fixed

LGTM. please change the return value to const std::vector& before landing. Also the dtor isn't necessary.
Comment 10 Antonio Gomes 2012-08-15 09:06:39 PDT
(In reply to comment #9)
> (From update of attachment 158413 [details])
> LGTM. please change the return value to const std::vector& before landing. Also the dtor isn't necessary.

Committed http://trac.webkit.org/changeset/125678