Bug 91607

Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs?

(In reply to comment #1) > Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs? Does this mean if they will have "hidden": true? If yes, then they will be marked as "hidden".

(In reply to comment #2) > (In reply to comment #1) > > Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs? > > Does this mean if they will have "hidden": true? If yes, then they will be marked as "hidden". Yes that's exactly my question was. Thank you.

Created attachment 153016 [details] Patch

Comment on attachment 153016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153016&action=review > Source/WebCore/inspector/InspectorCSSAgent.cpp:794 > + Document* document = m_domAgent->document(); You are currently getting flows from the main frame only. What if the page uses iframes or frame sets? You probably should pass context node id (from DOMAgent terms) into this method to get the flows for a given document.

Created attachment 153250 [details] Patch

Comment on attachment 153250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153250&action=review > LayoutTests/ChangeLog:3 > + Deleted tab space Please remove git comments from here. > LayoutTests/ChangeLog:16 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Nuke this line > LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:25 > + function test() { You probably should pick another name in order to not reuse "test" > LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:26 > + function namedFlowCallback(namedFlows) { Please place { on the next line > LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:31 > + InspectorTest.completeTest(); missing return; below. > LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:42 > + function documentCallback(document) { { on the next line. > Source/WebCore/ChangeLog:2 > + Web Inspector: Protocol Extension: add getNamedFlowCollection command missing blank line above > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:48 > + return m_namedFlows; Exposing private member for read-write does not sound good. > Source/WebCore/inspector/Inspector.json:2294 > + { "name": "documentNodeId", "$ref": "DOM.NodeId", "description": "The document node id for which to get the Named Flow Collection."} we typically call node ids "nodeId" > Source/WebCore/inspector/InspectorCSSAgent.cpp:796 > + if (!node || !(node->isDocumentNode())) { You should add InspectorDOMNode::assertDocumentNode instead. > Source/WebCore/inspector/InspectorCSSAgent.cpp:804 > + WebKitNamedFlowCollection::NamedFlowSet& namedFlowSet = document->namedFlows()->namedFlows(); So you could make flow collection return array of strings instead of exposing the private object. > Source/WebCore/inspector/InspectorCSSAgent.cpp:807 > + if ((*it)->flowState() == WebKitNamedFlow::FlowStateNull) This check could also be a part of the collection itself.

(In reply to comment #7) > > Source/WebCore/inspector/InspectorCSSAgent.cpp:796 > > + if (!node || !(node->isDocumentNode())) { > > You should add InspectorDOMNode::assertDocumentNode instead. I can't find "InspectorDOMNode". Did you meant "Node"?

(In reply to comment #8) > (In reply to comment #7) > > > Source/WebCore/inspector/InspectorCSSAgent.cpp:796 > > > + if (!node || !(node->isDocumentNode())) { > > > > You should add InspectorDOMNode::assertDocumentNode instead. > > I can't find "InspectorDOMNode". Did you meant "Node"? Sorry, I meant introduce InspectorDOMAgent::assertDocument as in InspectorDOMAgent::assertNode / InspectorDOMAgent::assertElement.

Created attachment 153444 [details] Patch

Comment on attachment 153444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153444&action=review > Source/WebCore/dom/WebKitNamedFlowCollection.h:33 > +#include <InspectorBackendDispatcher.h> You should not make arbitrary component depend on the inspector guts. Also, these are not visible in case of !ENABLED(INSPECTOR). You should return a Vector<String> instead.

Created attachment 153473 [details] Patch

Comment on attachment 153473 [details] Patch Attachment 153473 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13309224

Comment on attachment 153473 [details] Patch Attachment 153473 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13313174

Created attachment 153480 [details] Patch

Comment on attachment 153480 [details] Patch Clearing flags on attachment: 153480 Committed r123205: <http://trac.webkit.org/changeset/123205>

All reviewed patches have been landed. Closing bug.