Bug 93983 – [BlackBerry] Robust-fy the LayerWebKitThread ownership with InRegionScroller

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Antonio Gomes]

That ensure we have a thread safe cache of it

It came from a offline discussion with Arvid.

[Antonio Gomes]

Created attachment 158412 [details]
patch

[WebKit Review Bot]

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.

[Antonio Gomes]

Created attachment 158413 [details]
patch - style issue fixed

[Yong Li]

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?

[Antonio Gomes]

(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?

[Antonio Gomes]

> > > Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:45
> > > +    m_cachedCompositedScrollableLayer = 0;
> > 
> > Why is this needed?

Not all layers support composited scrolling.

[Antonio Gomes]

(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?

[Yong Li]

(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

[Yong Li]

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.

[Antonio Gomes]

(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