Bug 117833 – Web Inspector: Integrate new regionOversetChange event into inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Radu Stavila]

Integrate new regionOversetChange event into inspector

[Radar WebKit Bug Importer]

<rdar://problem/14215914>

[Radu Stavila]

Created attachment 205100 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 205100 [details]
Patch

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.

[Radu Stavila]

Created attachment 205181 [details]
Patch

[Radu Stavila]

Created attachment 205182 [details]
Patch (implemented changes requested by JoePeck)

[WebKit Commit Bot]

Comment on attachment 205182 [details]
Patch (implemented changes requested by JoePeck)

Clearing flags on attachment: 205182

Committed r151835: <http://trac.webkit.org/changeset/151835>