Summary: | Web Inspector: Metrics pane editing and visual feedback improvements | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2011-04-20 07:51:41 PDT
Created attachment 90344 [details]
[PATCH] Suggested solution
Comment on attachment 90344 [details] [PATCH] Suggested solution View in context: https://bugs.webkit.org/attachment.cgi?id=90344&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:94 > +void drawHighlightForBox(GraphicsContext& context, const FloatQuad& contentQuad, const FloatQuad& paddingQuad, const FloatQuad& borderQuad, const FloatQuad& marginQuad, const String& mode) mode -> enum > Source/WebCore/inspector/Inspector.json:900 > + { "name": "mode", "type": "string", "optional": true, "description": "The box model component(s) to highlight: \"content\", \"padding\", \"border\", \"margin\", \"all\" (default: \"all\")." }, "enum": ["content", "padding", ...] > Source/WebCore/inspector/InspectorDOMAgent.h:181 > + void highlight(ErrorString*, Node*, const String& mode = "all"); Do not use default values unless necessary. > Source/WebCore/inspector/front-end/MetricsSidebarPane.js:224 > + widthElement.addEventListener("click", this.startEditing.bind(this, widthElement, "width", "width", style), false); why did this change? > Source/WebCore/inspector/front-end/MetricsSidebarPane.js:262 > + WebInspector.highlightDOMNode(0, ""); mode should be optional. > Source/WebCore/inspector/front-end/inspector.js:320 > + DOMAgent.highlightDOMNode(nodeId, mode); mode || "all" Created attachment 90357 [details]
[PATCH] Comments addressed
(In reply to comment #2) > (From update of attachment 90344 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90344&action=review > > > Source/WebCore/inspector/DOMNodeHighlighter.cpp:94 > > +void drawHighlightForBox(GraphicsContext& context, const FloatQuad& contentQuad, const FloatQuad& paddingQuad, const FloatQuad& borderQuad, const FloatQuad& marginQuad, const String& mode) > > mode -> enum Done > > Source/WebCore/inspector/Inspector.json:900 > > + { "name": "mode", "type": "string", "optional": true, "description": "The box model component(s) to highlight: \"content\", \"padding\", \"border\", \"margin\", \"all\" (default: \"all\")." }, > > "enum": ["content", "padding", ...] Done > > Source/WebCore/inspector/InspectorDOMAgent.h:181 > > + void highlight(ErrorString*, Node*, const String& mode = "all"); > > Do not use default values unless necessary. Done > > Source/WebCore/inspector/front-end/MetricsSidebarPane.js:224 > > + widthElement.addEventListener("click", this.startEditing.bind(this, widthElement, "width", "width", style), false); > > why did this change? Inadvertent change, reverted > > Source/WebCore/inspector/front-end/MetricsSidebarPane.js:262 > > + WebInspector.highlightDOMNode(0, ""); > > mode should be optional. Done > > Source/WebCore/inspector/front-end/inspector.js:320 > > + DOMAgent.highlightDOMNode(nodeId, mode); > > mode || "all" Done Attachment 90357 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/DOMNodeHighlighter.h:47: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r84481: <http://trac.webkit.org/changeset/84481> This is an upstream of Chromium issue http://code.google.com/p/chromium/issues/detail?id=77773 http://trac.webkit.org/changeset/84481 might have broken Leopard Intel Debug (Tests) |