Bug 93443

Created attachment 158311 [details] Patch

Comment on attachment 158311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158311&action=review > Source/WebCore/inspector/InspectorCSSAgent.cpp:838 > + if (!m_namedFlowCollectionsRequested.contains(documentNodeId)) { I think m_namedFlowCollectionsRequested is superflous now that you have the more complete m_documentIdToNamedFlowSetMap. > Source/WebCore/inspector/InspectorCSSAgent.cpp:868 > + it = m_documentIdToNamedFlowSetMap.add(documentNodeId, NamedFlowSet()).iterator; You should register this flow in the m_documentIdToNamedFlowSetMap from getNamedFlowCollection as well. I think it is better to do that in buildObjectForNamedFlow. You collection tracks elements that are sent to the front-end and the buildObjectForNamedFlow is a perfect place to do that. > Source/WebCore/inspector/InspectorCSSAgent.cpp:1162 > + int documentNodeId = m_domAgent->boundNodeId(document); Looks like too much copypaste (add the "remove" parameter to the original method? reuse same implementation?)

(In reply to comment #2) > (From update of attachment 158311 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158311&action=review > > > Source/WebCore/inspector/InspectorCSSAgent.cpp:838 > > + if (!m_namedFlowCollectionsRequested.contains(documentNodeId)) { > > I think m_namedFlowCollectionsRequested is superflous now that you have the more complete m_documentIdToNamedFlowSetMap. I need a way to verify if the Named Flow Collection has been requested for a given document (to see if the NamedFlowCreated event should be dispatched). A solution would be to add a NULL pointer to the NamedFlowSet when the Named Flow Collection for some document is sent to the front-end: this will lead to a NamedFlowSet containing a single element.

> I need a way to verify if the Named Flow Collection has been requested for a given document (to see if the NamedFlowCreated event should be dispatched). > A solution would be to add a NULL pointer to the NamedFlowSet when the Named Flow Collection for some document is sent to the front-end: this will lead to a NamedFlowSet containing a single element. Not sure I follow. You get a request: getNamedFlowCollection, you return collection of flows for given document and populate m_documentIdToNamedFlowSetMap. If document had no flows, m_documentIdToNamedFlowSetMap would contain empty set for that document id. What do I miss?

(In reply to comment #4) > Not sure I follow. You get a request: getNamedFlowCollection, you return collection of flows for given document and populate m_documentIdToNamedFlowSetMap. If document had no flows, m_documentIdToNamedFlowSetMap would contain empty set for that document id. What do I miss? There are 2 situations: 1) getNamedFlowCollection is called for a given document: you dispatch all events related to all named flows from that document. 2) getFlowByName is called without calling getNamedFlowCollection: you only dispatch RegionLayoutUpdate and NamedFlowRemoved for the requested named flow. If on getNamedFlowCollection the NamedFlowSet is just filled with named flows you will not be able to tell if those flows were requested separately or if the entire collection has been requested. This information is used in order to decide if the front-end should be informed about a flow creation (the NamedFlowCreated event).

Ok, I understand it now. I was under the assumption that prior to calling getFlowByName, front-end would need to call getNamedFlowCollections in order to discover the names. So these two requests were overlapping for me. So there will be a UI where user would type in the flow name? I would suggest that you only leave getNamedFlowCollections. That would make front-end listen to created flows, but that is alright. As you mentioned, the traffic and cost of building the payloads is not high for these objects. Leaving only that method would simplify the backend code, reduce the API surface and reduce the overlap. What do you think?

(In reply to comment #6) > So there will be a UI where user would type in the flow name? I was thinking about dubroy's mockup: https://bug-90789-attachments.webkit.org/attachment.cgi?id=152749 > I would suggest that you only leave getNamedFlowCollections. That would make front-end listen to created flows, but that is alright. As you mentioned, the traffic and cost of building the payloads is not high for these objects. Leaving only that method would simplify the backend code, reduce the API surface and reduce the overlap. What do you think? It should be ok to remove the getFlowByName function. The front-end could keep a requested named flow collection in sync with the backend. In this case, the NamedFlowCreated and RegionLayoutUpdate events should send named flows, while NamedFlowRemoved should only send the name of the flow.

> I was thinking about dubroy's mockup: > https://bug-90789-attachments.webkit.org/attachment.cgi?id=152749 > I don't think sidebar is a good place for extensibility, you now have a drawer that has more real estate where user can focus on the flows. > It should be ok to remove the getFlowByName function. The front-end could keep a requested named flow collection in sync with the backend. In this case, the NamedFlowCreated and RegionLayoutUpdate events should send named flows, while NamedFlowRemoved should only send the name of the flow. I would proceed this way.

Created attachment 160934 [details] Patch

Comment on attachment 160934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160934&action=review Overall looks good. A number of renaming suggestions inline. > Source/WebCore/dom/NamedFlowCollection.cpp:98 > + InspectorInstrumentation::didRemoveNamedFlow(m_document, namedFlow); willRemoveNamedFlow? (it is now happening before the actual removal) > Source/WebCore/inspector/Inspector.json:2446 > + "name": "regionLayoutUpdate", regionLayoutUpdated? > Source/WebCore/inspector/InspectorCSSAgent.cpp:542 > + int documentNodeId = m_domAgent->boundNodeId(document); Sounds like this is checked 3 times. A helper method documentNodeWithRequestedFlowsId? > Source/WebCore/inspector/front-end/CSSStyleModel.js:381 > + if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowCreated)) It is too late for this check, it does not save anything. > Source/WebCore/inspector/front-end/CSSStyleModel.js:398 > if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowRemoved)) ditto > Source/WebCore/inspector/front-end/CSSStyleModel.js:415 > + if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.RegionLayoutUpdate)) ditto > Source/WebCore/inspector/front-end/CSSStyleModel.js:488 > + this._namedFlowCollections = {}; Should this fire an event? > Source/WebCore/inspector/front-end/CSSStyleModel.js:1436 > + getFlowByName: function(flowName) flowByName

(In reply to comment #10) > > Source/WebCore/inspector/front-end/CSSStyleModel.js:488 > > + this._namedFlowCollections = {}; > > Should this fire an event? I thought of it as a method to reset the cache when the document is updated, so I do not think it should fire an event.

Created attachment 161884 [details] Patch

Comment on attachment 161884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161884&action=review > Source/WebCore/rendering/RenderNamedFlowThread.cpp:340 > + if (!m_regionLayoutUpdateEventTimer.isActive()) Does not look like a part of this change. Web Inspector part lgtm, please make sure this one is fine with the code owners prior to landing.

Created attachment 161931 [details] Patch for landing

Comment on attachment 161931 [details] Patch for landing Rejecting attachment 161931 [details] from commit-queue. New failing tests: http/tests/cache/subresource-expiration-1.html http/tests/cache/stopped-revalidation.html Full output: http://queues.webkit.org/results/13732882

Comment on attachment 161931 [details] Patch for landing Rejecting attachment 161931 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: pyftpdlib/src --revision 977 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 96. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' Died at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2525. Full output: http://queues.webkit.org/results/13746440

Created attachment 162029 [details] Rebased Patch

Committed r127468: <http://trac.webkit.org/changeset/127468>

(In reply to comment #18) > Committed r127468: <http://trac.webkit.org/changeset/127468> This caused assertion failures - "crashes" on the ML Debug WK1 Tests: http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/492 http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r127468%20(493)/inspector/styles/protocol-css-regions-commands-crash-log.txt Please fix this soon or I will have to roll out this patch. Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef VM Regions Near 0xbbadbeef: --> __TEXT 000000010a816000-000000010a8b7000 [ 644K] r-x/rwx SM=COW /Volumes/VOLUME/* Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010c3216eb WebCore::Document::updateStyleIfNeeded() + 219 (Document.cpp:1901) 1 com.apple.WebCore 0x000000010c32192b WebCore::Document::updateLayout() + 155 (Document.cpp:1933) 2 com.apple.WebCore 0x000000010c321a82 WebCore::Document::updateLayoutIgnorePendingStylesheets() + 210 (Document.cpp:1969) 3 com.apple.WebCore 0x000000010d7c2dc6 WebCore::WebKitNamedFlow::getContent() + 118 (WebKitNamedFlow.cpp:164) 4 com.apple.WebCore 0x000000010c8c38e4 WebCore::InspectorCSSAgent::buildObjectForNamedFlow(WTF::String*, WebCore::WebKitNamedFlow*, int) + 68 (InspectorCSSAgent.cpp:1111) 5 com.apple.WebCore 0x000000010c8c3c25 WebCore::InspectorCSSAgent::didUpdateRegionLayout(WebCore::Document*, WebCore::WebKitNamedFlow*) + 117 (InspectorCSSAgent.cpp:566) 6 com.apple.WebCore 0x000000010c928720 WebCore::InspectorInstrumentation::didUpdateRegionLayoutImpl(WebCore::InstrumentingAgents*, WebCore::Document*, WebCore::WebKitNamedFlow*) + 64 (InspectorInstrumentation.cpp:250) 7 com.apple.WebCore 0x000000010d233d5e WebCore::InspectorInstrumentation::didUpdateRegionLayout(WebCore::Document*, WebCore::WebKitNamedFlow*) + 78 (InspectorInstrumentation.h:591) 8 com.apple.WebCore 0x000000010d23301b WebCore::RenderNamedFlowThread::dispatchRegionLayoutUpdateEvent() + 75 (RenderNamedFlowThread.cpp:342) 9 com.apple.WebCore 0x000000010d1a2c25 WebCore::RenderFlowThread::layout() + 1429 (RenderFlowThread.cpp:196) 10 com.apple.WebCore 0x000000010c5b3326 WebCore::RenderObject::layoutIfNeeded() + 54 (RenderObject.h:655) 11 com.apple.WebCore 0x000000010c5b28a6 WebCore::FlowThreadController::layoutRenderNamedFlowThreads() + 1126 (FlowThreadController.cpp:134) 12 com.apple.WebCore 0x000000010d354aaa WebCore::RenderView::layout() + 1066 (RenderView.cpp:159) 13 com.apple.WebCore 0x000000010c63f2db WebCore::FrameView::layout(bool) + 3147 (FrameView.cpp:1182) 14 com.apple.WebCore 0x000000010c63b1a0 WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView>*) + 32 (FrameView.cpp:2105) 15 com.apple.WebCore 0x000000010c6563b3 WebCore::Timer<WebCore::FrameView>::fired() + 115 (Timer.h:100) 16 com.apple.WebCore 0x000000010d71f37d WebCore::ThreadTimers::sharedTimerFiredInternal() + 285 (ThreadTimers.cpp:118) 17 com.apple.WebCore 0x000000010d71f119 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:94) 18 com.apple.WebCore 0x000000010d460553 WebCore::timerFired(__CFRunLoopTimer*, void*) + 67 (SharedTimerMac.mm:167)

(In reply to comment #19) > (In reply to comment #18) > > Committed r127468: <http://trac.webkit.org/changeset/127468> > > This caused assertion failures - "crashes" on the ML Debug WK1 Tests: > > http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/492 > http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r127468%20(493)/inspector/styles/protocol-css-regions-commands-crash-log.txt I reverted r127468 in <http://trac.webkit.org/r127500>.

Created attachment 162236 [details] Patch

Comment on attachment 162236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162236&action=review > Source/WebCore/inspector/front-end/CSSStyleModel.js:1403 > + for (var i = 0; i < payload.length; ++i) { This use without any preliminary checks is going to fail, given that you declare payload as nullable and optional (any reason for it to be both?) From the code, it seems that payload can be neither null, nor undefined (unless null comes in from the backend). > Source/WebCore/inspector/front-end/CSSStyleModel.js:1434 > + if (namedFlow === undefined) According to the WebKit coding guidelines, this should be "if (!namedFlow)"

Created attachment 162247 [details] Patch

Comment on attachment 162247 [details] Patch Clearing flags on attachment: 162247 Committed r127700: <http://trac.webkit.org/changeset/127700>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 95941

Compiling "sdk" Source/WebCore/inspector/front-end/CSSStyleModel.js:226: ERROR - variable error is undeclared if (error || !namedFlowMap) ^ 1 error(s), 0 warning(s)

Please use Source/WebCore/inspector/compile-front-end.py script to check for compilation errors before landing the patch. Inspector currently uses closure compiler revision 1810: http://mvnrepository.com/artifact/com.google.javascript/closure-compiler/r1810

Reverted: Committed r127703: <http://trac.webkit.org/changeset/127703>

Created attachment 162476 [details] Patch

Created attachment 162526 [details] Patch for landing

Comment on attachment 162526 [details] Patch for landing Clearing flags on attachment: 162526 Committed r127780: <http://trac.webkit.org/changeset/127780>

All reviewed patches have been landed. Closing bug.

FYI this patch broke closure compilation again, I have landed a fix. http://trac.webkit.org/changeset/127859

Re-opened since this is blocked by 96102

Created attachment 163066 [details] Patch This patch does not add any errors or warnings to the output of the compile-front-end.py script. Used this closure compiler: http://code.google.com/p/closure-compiler/

Does it fix test timeouts?

Created attachment 163559 [details] Patch

Comment on attachment 163559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163559&action=review The change looks good, but I have a CSSStyleModel API concern now that getNamedFlowCollectionAsync() is made use of. > Source/WebCore/inspector/InspectorCSSAgent.cpp:274 > + namedFlows.append(std::make_pair(it->first, it->second)); You need to double-check, but seemingly, namedFlows.append(*it); will have the same effect. > Source/WebCore/inspector/front-end/CSSStyleModel.js:180 > + getNamedFlowCollectionAsync: function(documentNodeId, userCallback) The method is named getNamedFlowCollectionAsync, however its result is "namedFlowCollection.namedFlowMap". Looking at its sole usage (in the real code), I guess it would be more appropriate to return namedFlowCollection to the userCallback (e.g. you wouldn't have to "this._namedFlowCollections[documentNodeId].flowByName(flowName)" in getFlowByNameAsync but rather "namedFlowCollection.flowByName(flowName)") - otherwise the returned result is just used to check for errors and discarded afterwards.

Created attachment 163580 [details] Patch

Comment on attachment 163580 [details] Patch r=me

Comment on attachment 163580 [details] Patch Clearing flags on attachment: 163580 Committed r128293: <http://trac.webkit.org/changeset/128293>

All reviewed patches have been landed. Closing bug.