Bug 117833

Summary: Web Inspector: Integrate new regionOversetChange event into inspector
Product: WebKit Reporter: Radu Stavila <stavila>
Component: Web InspectorAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, graouts, joepeck, mihnea, timothy, webkit-bug-importer, WebkitBugTracker
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 117508    
Attachments:
Description Flags
Patch
joepeck: review+
Patch
abucur: commit-queue+
Patch (implemented changes requested by JoePeck) none

Radu Stavila
Reported 2013-06-20 06:50:43 PDT
Integrate new regionOversetChange event into inspector
Attachments
Patch (21.05 KB, patch)
2013-06-20 09:54 PDT, Radu Stavila 		joepeck: review+
DetailsFormatted DiffDiff
Patch (20.49 KB, patch)
2013-06-21 07:00 PDT, Radu Stavila 		abucur: commit-queue+
DetailsFormatted DiffDiff
Patch (implemented changes requested by JoePeck) (20.49 KB, patch)
2013-06-21 07:08 PDT, Radu Stavila 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-06-20 06:51:07 PDT
<rdar://problem/14215914>
Radu Stavila
Comment 2 2013-06-20 09:54:31 PDT
Created attachment 205100 [details] Patch
Joseph Pecoraro
Comment 3 2013-06-20 18:16:26 PDT
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
Comment 4 2013-06-21 07:00:26 PDT
Created attachment 205181 [details] Patch
Radu Stavila
Comment 5 2013-06-21 07:08:11 PDT
Created attachment 205182 [details] Patch (implemented changes requested by JoePeck)
WebKit Commit Bot
Comment 6 2013-06-21 07:31:52 PDT
Comment on attachment 205182 [details] Patch (implemented changes requested by JoePeck) Clearing flags on attachment: 205182 Committed r151835: <http://trac.webkit.org/changeset/151835>
Note You need to log in before you can comment on or make changes to this bug.