Summary: | [BlackBerry] Ensure InRegionScrollableArea is valid before using. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Lattanzio <mlattanzio> | ||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mifenton, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mike Lattanzio
2012-12-12 12:10:37 PST
Created attachment 179099 [details]
Patch
Comment on attachment 179099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179099&action=review > Source/WebKit/blackberry/Api/InRegionScroller.cpp:420 > +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread) const > Source/WebKit/blackberry/Api/InRegionScroller.cpp:432 > +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node) const > Source/WebKit/blackberry/Api/InRegionScroller.cpp:437 > + for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++) IIRC calculateActiveAndShrinkCachedScrollableAreas shrink this vector. Does it need to be a for loop? (In reply to comment #2) > (From update of attachment 179099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179099&action=review > > > Source/WebKit/blackberry/Api/InRegionScroller.cpp:420 > > +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread) > > const > > > Source/WebKit/blackberry/Api/InRegionScroller.cpp:432 > > +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node) > > const > Fixing those. Good eye. > > Source/WebKit/blackberry/Api/InRegionScroller.cpp:437 > > + for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++) > > IIRC calculateActiveAndShrinkCachedScrollableAreas shrink this vector. Does it need to be a for loop? I think so. That shrinking is performed after we cast and use the layer, but we need to know if the layer still exists first. Created attachment 179123 [details]
Patch
Comment on attachment 179123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179123&action=review Please reupload with "Reviewed by antonio Gomes" and just request cq to land it. r+, some nits though. > Source/WebKit/blackberry/Api/InRegionScroller.cpp:420 > +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread) const WebCore:: is needed? > Source/WebKit/blackberry/Api/InRegionScroller.cpp:428 > + for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++) > + if (static_cast<InRegionScrollableArea*>(m_activeInRegionScrollableAreas[i])->cachedScrollableLayer() == layerWebKitThread) > + return true; > + for need {} ++i is preferred. > Source/WebKit/blackberry/Api/InRegionScroller.cpp:432 > +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node) const WebCore is needed? > Source/WebKit/blackberry/Api/InRegionScroller.cpp:439 > + for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++) > + if (static_cast<InRegionScrollableArea*>(m_activeInRegionScrollableAreas[i])->cachedScrollableNode() == node) > + return true; ditto ++i is preferred. Created attachment 179130 [details]
Patch
Comment on attachment 179130 [details]
Patch
LGTM.
Comment on attachment 179130 [details] Patch Clearing flags on attachment: 179130 Committed r137532: <http://trac.webkit.org/changeset/137532> All reviewed patches have been landed. Closing bug. |