[webkit-reviews] review granted: [Bug 117833] Web Inspector: Integrate new regionOversetChange event into inspector : [Attachment 205100] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Radu Stavila
<stavila at adobe.com>'s request for review:
Bug 117833: Web Inspector: Integrate new regionOversetChange event into
inspector
https://bugs.webkit.org/show_bug.cgi?id=117833

Attachment 205100: Patch
https://bugs.webkit.org/attachment.cgi?id=205100&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=205100&action=review


Change looks good to me, but could you explain the extra complexity in
ChangeRegionOversetTask? That matches existing behavior, but seems unnecessary.


> Source/WebCore/inspector/InspectorCSSAgent.cpp:310
> +: m_cssAgent(cssAgent)
> +, m_timer(this, &ChangeRegionOversetTask::onTimer)

Style: These lines should be indented.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:352
> +    Vector<std::pair<WebKitNamedFlow*, int> > namedFlows;
> +    
> +    for (HashMap<WebKitNamedFlow*, int>::iterator it = m_namedFlows.begin(),
end = m_namedFlows.end(); it != end; ++it)
> +	   namedFlows.append(std::make_pair(it->key, it->value));
> +    
> +    for (unsigned i = 0, size = namedFlows.size(); i < size; ++i) {
> +	   WebKitNamedFlow* namedFlow = namedFlows.at(i).first;
> +	   int documentNodeId = namedFlows.at(i).second;
> +	   
> +	   if (m_namedFlows.contains(namedFlow)) {
> +	       m_cssAgent->regionOversetChanged(namedFlow, documentNodeId);
> +	       m_namedFlows.remove(namedFlow);
> +	   }
> +    }
> +    
> +    if (!m_namedFlows.isEmpty() && !m_timer.isActive())
> +	   m_timer.startOneShot(0);

Why the extra copy to a vector first? Under what circumstances would
m_namedFlow be non-empty at the end?

Could this be simplified to just the following, or is there something I'm
missing:

    for (HashMap<WebKitNamedFlow*, int>::iterator it = m_namedFlows.begin(),
end = m_namedFlows.end(); it != end; ++it)
	m_cssAgent->regionOversetChanged(it->key, it->value);
    m_namedFlows.clear();

I see you are copying the style of UpdateRegionLayoutTest, but I'd have the
same question for that.