Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
237313003:
CSS shapes support in Web Inspector (Closed)
|
Created:
6 years, 8 months ago by Habib Virji Modified:
6 years, 7 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, dsinclair, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, leviw+renderwatch, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, zoltan1, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, jchaffraix+rendering, devtools-reviews_chromium.org, bemjb+rendering_chromium.org, pdr., aandrey+blink_chromium.org, rune+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionCSS shapes support in Web Inspector
Highlights the shape outside element when selected in web inspector:
- Compute shape information to display in inspector overlay canvas
- Path is computed based on ShapeOutsideInfo.
- In Inspector view only highlighted shape and margin are shown, no quad node is highlighted.
R=jochen@chromium.org, betravis@adobe.com
BUG=351441
TEST=Different shapes tests in the web inspector
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173379
Patch Set 1 #
Total comments: 21
Patch Set 2 : Code review comment and add test #
Total comments: 13
Patch Set 3 : Code review comments fixes #
Total comments: 2
Patch Set 4 : Corrected wrong if statement condition #
Total comments: 23
Patch Set 5 : Updated to latest master #Patch Set 6 : Code review and updated to latest master #
Total comments: 4
Patch Set 7 : Code review comments update #
Total comments: 8
Patch Set 8 : Addressed review comment #
Total comments: 2
Patch Set 9 : Updated test to use inspector protocol instead of internals #
Total comments: 2
Patch Set 10 : Updated inspectorOverlay to not pass string instead pass JSONObject, which is serialized #
Total comments: 6
Patch Set 11 : Use TypeBuilder::DOM::ShapeOutsideInfo in InspectorDOMAgent.cpp #
Total comments: 12
Patch Set 12 : Combined buildObjectForShapeOutside static and member function, and made setShapeOutside optional #
Total comments: 6
Patch Set 13 : Removed JSONObject created in buildObjectForShapeOutsideInfo. Storing directly in type builder #
Total comments: 19
Patch Set 14 : Instead of converion using runtime cast to convert JSONArray to TypeBuilder #
Total comments: 10
Patch Set 15 : Removed runtime cast, instead using TypeBuilder array #
Total comments: 4
Patch Set 16 : Code review comments. #
Total comments: 3
Patch Set 17 : Code review comments #
Total comments: 3
Messages
Total messages: 52 (0 generated)
+pfeldman
https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:525: PathApplyInfo* pathApplyInfo = static_cast<PathApplyInfo*>(info); Please don't reinterpret, call it using simple loop instead. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:569: info.rootView = containingFrame->page()->mainFrame()->view(); Extract FrameView* mainView above - you use it multiple times. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:570: info.view = containingFrame->view(); FrameView* containingView https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:342: function pathCommand(context, commands, name, index, length) { { on the next line https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:343: context[name].apply(context, commands.slice(index + 1, index + length + 1)); We plan on annotating this code with JSDoc, reflection like this makes things harder to compile / lint. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:357: case 'M': We use double quotes for strings. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:358: commandsIndex = pathCommand(context, commands, "moveTo", commandsIndex, 2); Lets just call context.moveTo(...) and increment index by 2. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:600: function _drawShapeHighlight(shapeInfo) { { on the next line https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... File Source/core/rendering/shapes/Shape.h (right): https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... Source/core/rendering/shapes/Shape.h:80: virtual void buildDisplayPaths(DisplayPaths&) const = 0; Someone other than me would need to assess changes to rendering.
Looks quite good to me but found a few issues still. https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... File Source/core/rendering/shapes/PolygonShape.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... Source/core/rendering/shapes/PolygonShape.cpp:157: for (size_t i = 1; i < m_polygon.numberOfVertices(); i++) the prefix ++ is preferred. Depending on the compiler it is either quicker or equally quick as the postfix, so there is not much risk in using the prefix one always for simple for loops. https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... File Source/core/rendering/shapes/RectangleShape.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... Source/core/rendering/shapes/RectangleShape.cpp:93: paths.shape.addRoundedRect(m_bounds, FloatSize(m_radii.width(), m_radii.height())); Looks like m_radii is a FloatSize, so you can just pass it.
@pfeldman and @rwlbuis: updated as per review comments and added tests. Only thing did not addressed was avoid usage of static_cast and that was because it is existing function that accepts void and thus need to use static cast. Let me know if anything further to look into. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:525: PathApplyInfo* pathApplyInfo = static_cast<PathApplyInfo*>(info); Tried to remove but problem is paths.shape.apply(&info, &appendPathSegment) below is accepting first argument void and second argument as callback function. Apply function sets the PathElement value, thus need for the static cast. Suggestion to loop instead of static cast was not clear to me. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:569: info.rootView = containingFrame->page()->mainFrame()->view(); On 2014/04/15 11:59:09, pfeldman wrote: > Extract FrameView* mainView above - you use it multiple times. Done. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:570: info.view = containingFrame->view(); On 2014/04/15 11:59:09, pfeldman wrote: > FrameView* containingView Done. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:342: function pathCommand(context, commands, name, index, length) { On 2014/04/15 11:59:09, pfeldman wrote: > { on the next line Done. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:358: commandsIndex = pathCommand(context, commands, "moveTo", commandsIndex, 2); On 2014/04/15 11:59:09, pfeldman wrote: > Lets just call context.moveTo(...) and increment index by 2. Done. https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlayPage.html:600: function _drawShapeHighlight(shapeInfo) { On 2014/04/15 11:59:09, pfeldman wrote: > { on the next line Done. https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... File Source/core/rendering/shapes/PolygonShape.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... Source/core/rendering/shapes/PolygonShape.cpp:157: for (size_t i = 1; i < m_polygon.numberOfVertices(); i++) On 2014/04/15 15:48:21, rwlbuis wrote: > the prefix ++ is preferred. Depending on the compiler it is either quicker or > equally quick as the postfix, so there is not much risk in using the prefix one > always for simple for loops. Done. https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... File Source/core/rendering/shapes/RectangleShape.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes... Source/core/rendering/shapes/RectangleShape.cpp:93: paths.shape.addRoundedRect(m_bounds, FloatSize(m_radii.width(), m_radii.height())); On 2014/04/15 15:48:21, rwlbuis wrote: > Looks like m_radii is a FloatSize, so you can just pass it. Done.
On 2014/04/17 15:43:47, Habib Virji wrote: > @pfeldman and @rwlbuis: updated as per review comments and added tests. Great! I noticed a lot of new files were added in the new patchset, what are they all doing? If it adds behavior you will want to put it in the Description.
https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:525: PathApplyInfo* pathApplyInfo = static_cast<PathApplyInfo*>(info); On 2014/04/17 15:43:47, Habib Virji wrote: > Tried to remove but problem is paths.shape.apply(&info, &appendPathSegment) > below is accepting first argument void and second argument as callback function. > Apply function sets the PathElement value, thus need for the static cast. > Suggestion to loop instead of static cast was not clear to me. Well, the current code is along the lines of what I can see in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., so I believe this approach is fine. Pavel? https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorOverlay.cpp:530: appendPathCommandAndPoints(pathApplyInfo, ("M"), pathElement->points, 1); The parentheses around strings are not needed? https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:349: while(commandsIndex < commandsLength) { Whitespace between "while" and parenthesis https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:350: switch(commands[commandsIndex]) { Whitespace between "switch" and parenthesis https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:392: if (!(shapeInfo.shape || shapeInfo.marginShape)) optional: if (!shapeInfo.shape && !shapeInfo.marginShape) might or might not be more readable (one pair of parentheses less) https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... File Source/core/rendering/shapes/RasterShape.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... Source/core/rendering/shapes/RasterShape.cpp:123: for (int y = bounds().y(); y < bounds().maxY(); y++) { You've got two loops using bounds().maxY() in the termination condition, so depending on the compiler, you might want to extract it into a local variable to avoid recalculating it at each iteration. https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... Source/core/rendering/shapes/RasterShape.cpp:128: int endY = y + 1; This could easily go into the for-initializer https://codereview.chromium.org/237313003/diff/40001/Source/core/testing/Inte... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/testing/Inte... Source/core/testing/Internals.cpp:823: return object? object->toJSONString(): String(); whitespace before "?"
On 2014/04/17 16:03:51, rwlbuis wrote: > On 2014/04/17 15:43:47, Habib Virji wrote: > > @pfeldman and @rwlbuis: updated as per review comments and added tests. > > Great! I noticed a lot of new files were added in the new patchset, what are > they all doing? If it adds behavior you will want to put it in the > Description. They are just for getting highlightShape details in test script, since they are not available in the test environment. No new functionality in this patch
https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... File Source/core/rendering/shapes/RasterShape.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... Source/core/rendering/shapes/RasterShape.cpp:128: int endY = y + 1; On 2014/04/17 16:11:32, apavlov wrote: > This could easily go into the for-initializer Sorry, it's not visible outside the loop, unlike in JavaScript :)
@apavlov: thanks for review. Corrected the changes suggested https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:349: while(commandsIndex < commandsLength) { On 2014/04/17 16:11:32, apavlov wrote: > Whitespace between "while" and parenthesis Done. https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:350: switch(commands[commandsIndex]) { On 2014/04/17 16:11:32, apavlov wrote: > Whitespace between "switch" and parenthesis Done. https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:392: if (!(shapeInfo.shape || shapeInfo.marginShape)) On 2014/04/17 16:11:32, apavlov wrote: > optional: > > if (!shapeInfo.shape && !shapeInfo.marginShape) > > might or might not be more readable (one pair of parentheses less) Done. https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:392: if (!(shapeInfo.shape || shapeInfo.marginShape)) On 2014/04/17 16:11:32, apavlov wrote: > optional: > > if (!shapeInfo.shape && !shapeInfo.marginShape) > > might or might not be more readable (one pair of parentheses less) Done. https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... File Source/core/rendering/shapes/RasterShape.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/sh... Source/core/rendering/shapes/RasterShape.cpp:123: for (int y = bounds().y(); y < bounds().maxY(); y++) { On 2014/04/17 16:11:32, apavlov wrote: > You've got two loops using bounds().maxY() in the termination condition, so > depending on the compiler, you might want to extract it into a local variable to > avoid recalculating it at each iteration. Done. https://codereview.chromium.org/237313003/diff/40001/Source/core/testing/Inte... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/testing/Inte... Source/core/testing/Internals.cpp:823: return object? object->toJSONString(): String(); On 2014/04/17 16:11:32, apavlov wrote: > whitespace before "?" Done.
https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:392: if (!shapeInfo.shape || !shapeInfo.marginShape) This is wrong: !(a || b) === (!a && !b). See http://en.wikipedia.org/wiki/De_Morgan's_laws for the reference.
Thanks for pointing out. corrected now. https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:392: if (!shapeInfo.shape || !shapeInfo.marginShape) On 2014/04/17 16:39:12, apavlov wrote: > This is wrong: !(a || b) === (!a && !b). See > http://en.wikipedia.org/wiki/De_Morgan for the reference. Thanks for pointing out. Corrected
On 2014/04/17 16:43:15, Habib Virji wrote: > Thanks for pointing out. Corrected Thanks for fixing the style, Habib! Deferring to the original reviewers re the code logic.
I'm excited to see this patch making progress. The code looks pretty solid to me, my main concern is over test coverage. You may want to break out the untested features (shape margin, polygons and raster shapes) or just add additional tests. Would also like to see the results of running this on the try bots, as different platforms had some issues which required a more robust set of tests. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/elements-test.js:762: output("command: moveTo, shapes point are: "+ point1 + " " + point2); Is this long form more useful than just the SVG-style path syntax? It seems to generate a lot of text. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... File LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt:3: bezierCurveTo : 172.000000, 3.000000, 221.000000, 52.000000, 221.000000, 112.000000lineTo: 221.000000 , 112.000000 I'm not as big a fan of this style of test rather than writing a JS test that compares two paths (you'd basically pass in the path you expect to find in the test case and assert they are equal). When you get into serious curved shapes, the control points will not be even numbers like these, and therefore the numbers should have a little tolerance, especially across platforms. You may also run into trouble with native implementations of path rectangles that serialize commands slightly differently (closing or not closing their last segment). https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... File LayoutTests/inspector/elements/highlight-css-shapes-outside.html (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside.html:7: .float { I don't see test cases for polygons, rasters, or shape-margin. I would either test them or separate out the added functionality into a subsequent patch. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside.html:63: function nodeSelectedContentBox() Can you generalize this so you don't need so many declared functions? All you're doing is "select id and call dumpInspectorHighlight", which you should be able to simplify down to an array of test cases and a function or two. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:342: function drawCSSPath(commands, fillColor) The name CSSPath seems a little odd, as I don't know of any CSS path element. This particular syntax is borrowed from SVG. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:353: commandsIndex += 2 + 1; Why not just combine these (ie commandsIndex += 3)? It seems extraneous to add both numbers. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:640: } else { Are you not drawing the box model highlights when a shape is present? In WK, the shape highlight is drawn on top of the box model highlight, which I think provides additional useful information. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:679: Added line break. https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... File Source/core/rendering/shapes/PolygonShape.cpp (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... Source/core/rendering/shapes/PolygonShape.cpp:158: paths.shape.closeSubpath(); Is there not support for displaying polygon shape margins yet? https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... File Source/core/rendering/shapes/ShapeOutsideInfo.h (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... Source/core/rendering/shapes/ShapeOutsideInfo.h:119: Added line break. There's another one below. It's probably a fine stylistic change, but generally I try not to touch anything if it's not directly related to the patch. https://codereview.chromium.org/237313003/diff/80001/Source/core/testing/Inte... File Source/core/testing/Internals.cpp (left): https://codereview.chromium.org/237313003/diff/80001/Source/core/testing/Inte... Source/core/testing/Internals.cpp:29: Removed line break.
Apologise for delay in submitting changes, we had holiday on Friday and Monday. Yesterday ran into problem with latest master not compatible with my changes as suggested by Rob. Polygon margin is still missing and this is due to lot of code to calculate FloatPolygon will be required. Lot of porting will be required from WK to get it working. Let me know or can submit it as a separate CL. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/elements-test.js:762: output("command: moveTo, shapes point are: "+ point1 + " " + point2); On 2014/04/17 20:26:45, Bear Travis wrote: > Is this long form more useful than just the SVG-style path syntax? It seems to > generate a lot of text. Done. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... File LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt:3: bezierCurveTo : 172.000000, 3.000000, 221.000000, 52.000000, 221.000000, 112.000000lineTo: 221.000000 , 112.000000 Have tried few changes in elements-test.js and it print less cryptic data. Should i rework on assert way or print in SVG-path style? If you still feel assert as in LayoutTests/fast/shapes/shape-outside let me know will try to rework on the test. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... File LayoutTests/inspector/elements/highlight-css-shapes-outside.html (right): https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside.html:7: .float { On 2014/04/17 20:26:45, Bear Travis wrote: > I don't see test cases for polygons, rasters, or shape-margin. I would either > test them or separate out the added functionality into a subsequent patch. Done. Added polygon and rasters. Shape-margin is already there combined with all shapes, do you want to see them as two separate test. https://codereview.chromium.org/237313003/diff/80001/LayoutTests/inspector/el... LayoutTests/inspector/elements/highlight-css-shapes-outside.html:63: function nodeSelectedContentBox() On 2014/04/17 20:26:45, Bear Travis wrote: > Can you generalize this so you don't need so many declared functions? All you're > doing is "select id and call dumpInspectorHighlight", which you should be able > to simplify down to an array of test cases and a function or two. Done. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:342: function drawCSSPath(commands, fillColor) On 2014/04/17 20:26:45, Bear Travis wrote: > The name CSSPath seems a little odd, as I don't know of any CSS path element. > This particular syntax is borrowed from SVG. Done. Wanted to be explicit, function usage details. Have added comment in top to be explicit it is for css shapes. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:353: commandsIndex += 2 + 1; On 2014/04/17 20:26:45, Bear Travis wrote: > Why not just combine these (ie commandsIndex += 3)? It seems extraneous to add > both numbers. Done. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:640: } else { On 2014/04/17 20:26:45, Bear Travis wrote: > Are you not drawing the box model highlights when a shape is present? In WK, the > shape highlight is drawn on top of the box model highlight, which I think > provides additional useful information. OK, I tried with the box model previously but found so many boxes confusing. Perhaps we need some way of indicating in 4 boxes what is what, and indicate clearly shape highlight. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:640: } else { On 2014/04/17 20:26:45, Bear Travis wrote: > Are you not drawing the box model highlights when a shape is present? In WK, the > shape highlight is drawn on top of the box model highlight, which I think > provides additional useful information. Done. https://codereview.chromium.org/237313003/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorOverlayPage.html:679: On 2014/04/17 20:26:45, Bear Travis wrote: > Added line break. Done. https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... File Source/core/rendering/shapes/PolygonShape.cpp (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... Source/core/rendering/shapes/PolygonShape.cpp:158: paths.shape.closeSubpath(); On 2014/04/17 20:26:45, Bear Travis wrote: > Is there not support for displaying polygon shape margins yet? Margin does not work as it has lot of functionality missing as compared to WK missing such as shapeMarginBound and drawArc. Let me know if to move these functionality from WK. https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... File Source/core/rendering/shapes/ShapeOutsideInfo.h (right): https://codereview.chromium.org/237313003/diff/80001/Source/core/rendering/sh... Source/core/rendering/shapes/ShapeOutsideInfo.h:119: On 2014/04/17 20:26:45, Bear Travis wrote: > Added line break. There's another one below. It's probably a fine stylistic > change, but generally I try not to touch anything if it's not directly related > to the patch. Done. https://codereview.chromium.org/237313003/diff/80001/Source/core/testing/Inte... File Source/core/testing/Internals.cpp (left): https://codereview.chromium.org/237313003/diff/80001/Source/core/testing/Inte... Source/core/testing/Internals.cpp:29: On 2014/04/17 20:26:45, Bear Travis wrote: > Removed line break. Done.
FWIW LGTM with some small nits. You don't have to write assert-style tests unless you start needing to test if paths are approximately equal rather than just equal. I found that to be the case with curved paths and multiple platforms in WK, may not be an issue here. Exactly what to show when highlighting a shape is a bit of a design decision. In WK, we show everything (including the tooltip) and draw the shape + shape margin highlight on top. I think further iterations could add something like the box metrics panel that allowed you to toggle which highlights were showing. I like this, because the box metrics and tooltip information tends to be very useful, at least to me. But, my opinion here more so than anything else. You'll probably want to talk to the Blink Inspector team. https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/elements-test.js:758: for(var commandsIndex = 0; commandsIndex < commands.length;) { Is this different than just doing text = parse.shape.join(" ")? You could also do JSON.stringify(parse.shape) if you wanted the [] https://codereview.chromium.org/237313003/diff/110001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/110001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:162: Extra space character here.
Thanks Bear Travis. Addressed your comments. https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/elements-test.js:758: for(var commandsIndex = 0; commandsIndex < commands.length;) { On 2014/04/23 20:46:06, Bear Travis wrote: > Is this different than just doing text = parse.shape.join(" ")? > > You could also do JSON.stringify(parse.shape) if you wanted the [] Done. https://codereview.chromium.org/237313003/diff/110001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/110001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:162: On 2014/04/23 20:46:06, Bear Travis wrote: > Extra space character here. Done.
Devtools part looks good. Pavel, do you have any reservations on the patch?
Looks pretty good, found some small stuff. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorController.h (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorController.h:70: class RenderBox; Nit: looks like this is not needed (anymore)? https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:522: FloatPoint point; point is not used? https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.h (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.h:32: #include "core/rendering/RenderBoxModelObject.h" Needed? https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:593: if (!shapeInfo.shape && !shapeInfo.marginShape) You could combine above using else so you only test shapeInfo.shape once.
Thanks Rob, addressed your comments. @pavel: let me know if anything more is needed. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorController.h (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorController.h:70: class RenderBox; On 2014/04/24 15:17:27, rwlbuis wrote: > Nit: looks like this is not needed (anymore)? Done. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:522: FloatPoint point; On 2014/04/24 15:17:27, rwlbuis wrote: > point is not used? Done. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.h (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.h:32: #include "core/rendering/RenderBoxModelObject.h" On 2014/04/24 15:17:27, rwlbuis wrote: > Needed? Done. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:593: if (!shapeInfo.shape && !shapeInfo.marginShape) On 2014/04/24 15:17:27, rwlbuis wrote: > You could combine above using else so you only test shapeInfo.shape once. Done.
This looks great! The only question I have is test-related. Once it is addressed, we can land. https://codereview.chromium.org/237313003/diff/150001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/150001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/elements-test.js:752: function dumpInspectorHighlightShape() Testing highlight via internals was not the right thing to do, I'm fixing it here: https://codereview.chromium.org/257873002/. You should rebase on top of it and make shape a part of the BoxModel structure in the inspector protocol.
Updated to use inspector protocol and made it part of getBoxModel. Kept it separated from buildNodeQuads as it is used by buildNodeHighlight. https://codereview.chromium.org/237313003/diff/150001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/150001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/elements-test.js:752: function dumpInspectorHighlightShape() On 2014/04/25 12:31:01, pfeldman wrote: > Testing highlight via internals was not the right thing to do, I'm fixing it > here: https://codereview.chromium.org/257873002/. > > You should rebase on top of it and make shape a part of the BoxModel structure > in the inspector protocol. Done.
https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:764: shapeOutsideStr = shapeOutsideInfo ? shapeOutsideInfo->toJSONString() : String(); You should not serialize shape into a string, declare a DOM.ShapeOutsideInfo type in protocol.json so that it was an object with "bounds", "shape" and "marginShape" (or what else could it have?); use array of "*" as a type of "shape" and "marginShape" fields. It will generate a C++ class for you with these properties. That's what your buildObjectForShapeOutside will return. You can use this object both in overlay (via serialization) and return it over the protocol.
@pfeldman: Updated as suggested. https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:764: shapeOutsideStr = shapeOutsideInfo ? shapeOutsideInfo->toJSONString() : String(); On 2014/04/30 07:43:48, pfeldman wrote: > You should not serialize shape into a string, declare a DOM.ShapeOutsideInfo > type in protocol.json so that it was an object with "bounds", "shape" and > "marginShape" (or what else could it have?); use array of "*" as a type of > "shape" and "marginShape" fields. It will generate a C++ class for you with > these properties. That's what your buildObjectForShapeOutside will return. You > can use this object both in overlay (via serialization) and return it over the > protocol. Done.
https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1381: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) I'm not sure I follow what this does. It puts given value into an array and returns this array. But that's not how it is used below. https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1416: RefPtr<JSONObject> shapeOutsideObj = m_overlay->buildObjectForShapeOutside(node); I'm confused - why doesn't it return RefPtr<TypeBuilder::DOM::ShapeOutsideInfo>? https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... Source/devtools/protocol.json:1929: { "name": "bounds", "type": "array", "items": { "type": "any"}, "description": "Shape bounds" }, We have a Quad for this used in BoxModel above.
https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1381: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) On 2014/05/01 10:18:50, pfeldman wrote: > I'm not sure I follow what this does. It puts given value into an array and > returns this array. But that's not how it is used below. Idea is to take a JSONValue put in TypeBuilder::Array and return back as an argument for setBounds/setShape/setMarginShape. https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1416: RefPtr<JSONObject> shapeOutsideObj = m_overlay->buildObjectForShapeOutside(node); On 2014/05/01 10:18:50, pfeldman wrote: > I'm confused - why doesn't it return RefPtr<TypeBuilder::DOM::ShapeOutsideInfo>? Ok, i was not sure whether it is correct to use typeBuilder::DOM inside inspector overlay. Will look into it and update. https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... Source/devtools/protocol.json:1929: { "name": "bounds", "type": "array", "items": { "type": "any"}, "description": "Shape bounds" }, On 2014/05/01 10:18:50, pfeldman wrote: > We have a Quad for this used in BoxModel above. Ok, sorry missed it will rework and upload new code.
On 2014/05/01 10:18:49, pfeldman wrote: > https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... > File Source/core/inspector/InspectorDOMAgent.cpp (right): > > https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... > Source/core/inspector/InspectorDOMAgent.cpp:1381: static > RefPtr<TypeBuilder::Array<JSONValue> > > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) > I'm not sure I follow what this does. It puts given value into an array and > returns this array. But that's not how it is used below. > > https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/I... > Source/core/inspector/InspectorDOMAgent.cpp:1416: RefPtr<JSONObject> > shapeOutsideObj = m_overlay->buildObjectForShapeOutside(node); > I'm confused - why doesn't it return RefPtr<TypeBuilder::DOM::ShapeOutsideInfo>? > > https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... > File Source/devtools/protocol.json (right): > > https://codereview.chromium.org/237313003/diff/190001/Source/devtools/protoco... > Source/devtools/protocol.json:1929: { "name": "bounds", "type": "array", > "items": { "type": "any"}, "description": "Shape bounds" }, > We have a Quad for this used in BoxModel above. @pfeldman: Using ShapeOutsideInfo in inspectorOverlay requires converting JSONValue to TypeBuilder::DOM::ShapeOutsideInfo and then for nodeForHighlight convert from TypeBuilder::DOM::ShapeOutsideInfo to JSONValue for drawNodeHighlight only takes JSONVaue. Is it okay to do so many conversions?
> @pfeldman: Using ShapeOutsideInfo in inspectorOverlay requires converting > JSONValue to TypeBuilder::DOM::ShapeOutsideInfo and then for nodeForHighlight > convert from TypeBuilder::DOM::ShapeOutsideInfo to JSONValue for > drawNodeHighlight only takes JSONVaue. Is it okay to do so many conversions? Oh, it is that JSONObjectBase won't let you cast to JSONObject... Could you build ShapeOutsideInfo once and then send it over the protocol as is + send it to overlay as JSON-ified string for now? I'm thinking of inheriting generated types from JSONObject, not JSONObjectBase for convenience.
"Oh, it is that JSONObjectBase won't let you cast to JSONObject..." --> Yes that's the main hurdle and InspectorDOMAgent has quite few conversion similar to buildArrayForShapeOutside "Could you build ShapeOutsideInfo once and then send it over the protocol as is" --> Yes that's what I tried but issue ShapeOutsideInfo still need to be initialized and its parameter need to be converted "+ send it to overlay as JSON-ified string for now?" --> I have managed to get ShapeOutsideInfo object back to getBoxModel, but it is with all conversion. "I'm thinking of inheriting generated types from JSONObject, not JSONObjectBase for convenience." --> That will benefit a lot
> "Could you build ShapeOutsideInfo once and then send it over the protocol as > is" > --> Yes that's what I tried but issue ShapeOutsideInfo still need to be > initialized and its parameter need to be converted Could you elaborate on this one, I am not sure I completely follow.
" Could you elaborate on this one, I am not sure I completely follow." --> What I meant was declaration of object, ShapeOutsideInfo in protocol.json require initialization of this object(as done in buildObjectForShapeOutside). I could not find a way of sending it over the protocol without doing conversion from JSONObject to TypeBuilder. In protocol.json, if we declare as string previously then probably conversion would not be needed. "send to overlay as JSON-stringified string" --> If we define ShapeOutsideInfo as an object, then string received from Overlay will need to be parsed and object (ShapeOutsideInfo) needs to be initialized I suppose it will be same conversion. Converting from string to JSONValue and then from JSONValue initialize different elements of ShapeOutsideInfo object. I have uploaded new patch where buildShapeForShapeOutside returns TypeBuilder::DOM::ShapeOutsideInfo, it basically moves out stuff from InspectorDOMAgent to InspectorOveraly especially conversion and check parts. BTW i have not initialized bound element as it is not currently used and it is quite complex. buildObjectForShapeOutsideLocal is storing it as array, first it need to be converted to JSONValue and then from JSONValue it needs to be converted to FloatQuad to initialize setBound. I am working on it, but uploaded patch to get your consent if to keep ShapeOutsideInfo as object or change it to String.
This looks much better. One final update and it is good to go! https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:604: RefPtr<TypeBuilder::DOM::ShapeOutsideInfo> shapeObject = TypeBuilder::DOM::ShapeOutsideInfo::create() You don't need this, see below. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:610: if (renderer->isBox()) { // This check is necessary as shapeOutside is not mandatory part of box model:w Since shapeOutside is optional, lets start with if (!render-isBox() || !toRenderBox(renderer)->shapeOutside()) return 0; https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:614: RefPtr<JSONObject> value = buildObjectForShapeOutsideLocal(node); Now instead of doing this, I'd suggest that you inline buildObjectForShapeOutsideLocal here. I'll comment on its other call site below. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:618: .setBounds(buildArrayForQuadTypeBuilder(FloatQuad())) Why empty quad here? https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:689: RefPtr<JSONObject> shapeObject = buildObjectForShapeOutsideLocal(node); RefPtr<JSONObject> shapeObject = buildObjectForShapeOutside(node)->asObject(); // Turns out it is already available. https://codereview.chromium.org/237313003/diff/210001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/210001/Source/devtools/protoco... Source/devtools/protocol.json:1920: { "name": "shapeOutside", "$ref": "ShapeOutsideInfo", "description": "Shape outside co-ordinates" } "optional": true
Thanks pfeldman, updated as suggested. Particularly asObject convesion was really handy. Few extra checks to avoid issues for other tests. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:604: RefPtr<TypeBuilder::DOM::ShapeOutsideInfo> shapeObject = TypeBuilder::DOM::ShapeOutsideInfo::create() On 2014/05/02 07:21:54, pfeldman wrote: > You don't need this, see below. Done. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:610: if (renderer->isBox()) { // This check is necessary as shapeOutside is not mandatory part of box model:w On 2014/05/02 07:21:54, pfeldman wrote: > Since shapeOutside is optional, lets start with > > if (!render-isBox() || !toRenderBox(renderer)->shapeOutside()) > return 0; Done. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:614: RefPtr<JSONObject> value = buildObjectForShapeOutsideLocal(node); On 2014/05/02 07:21:54, pfeldman wrote: > Now instead of doing this, I'd suggest that you inline > buildObjectForShapeOutsideLocal here. I'll comment on its other call site below. Done. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:618: .setBounds(buildArrayForQuadTypeBuilder(FloatQuad())) On 2014/05/02 07:21:54, pfeldman wrote: > Why empty quad here? Done. https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:689: RefPtr<JSONObject> shapeObject = buildObjectForShapeOutsideLocal(node); On 2014/05/02 07:21:54, pfeldman wrote: > RefPtr<JSONObject> shapeObject = buildObjectForShapeOutside(node)->asObject(); > // Turns out it is already available. Done. https://codereview.chromium.org/237313003/diff/210001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/210001/Source/devtools/protoco... Source/devtools/protocol.json:1920: { "name": "shapeOutside", "$ref": "ShapeOutsideInfo", "description": "Shape outside co-ordinates" } On 2014/05/02 07:21:54, pfeldman wrote: > "optional": true Done.
https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:559: if (!renderer || !renderer->isBox() || !toRenderBox(renderer)->shapeOutsideInfo()) // This check is necessary as shapeOutside is not mandatory part of box model I don't think you need this comment. https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:572: shapeObject->setArray("bounds", buildArrayForQuad(shapeQuad)); This is unused. https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:600: .setShape(buildArrayForShapeOutside(shapeObject->getArray("shape"))) Why creating shapeObject first and fetching its property later? You should build single good ShapeOutsideInfo in this method.
Removed comments and JSONObject created. Only thing might be bounds, which I have kept as will be useful while creating object in inspector. It values though I am not testing in the test. https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:559: if (!renderer || !renderer->isBox() || !toRenderBox(renderer)->shapeOutsideInfo()) // This check is necessary as shapeOutside is not mandatory part of box model On 2014/05/02 10:12:07, pfeldman wrote: > I don't think you need this comment. Done. https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:572: shapeObject->setArray("bounds", buildArrayForQuad(shapeQuad)); On 2014/05/02 10:12:07, pfeldman wrote: > This is unused. On test, I am not using it but in the inspector side this is useful. https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:600: .setShape(buildArrayForShapeOutside(shapeObject->getArray("shape"))) On 2014/05/02 10:12:07, pfeldman wrote: > Why creating shapeObject first and fetching its property later? You should build > single good ShapeOutsideInfo in this method. Yep valid point. corrected.
I made one more path over inspector-related core and there is one last thing I was wondering in comment #24 that I am not sure is addressed. Could you comment? https://codereview.chromium.org/237313003/diff/250001/LayoutTests/inspector/e... File LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt (right): https://codereview.chromium.org/237313003/diff/250001/LayoutTests/inspector/e... LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt:1: [["M",88,8,"L",88,8,"C",132,8,168,44,168,88,"L",168,88,"C",168,132,132,168,88,168,"L",88,168,"C",44,168,8,132,8,88,"L",8,88,"C",8,44,44,8,88,8,"Z"]] So shape is array of arrays. This does not sound right. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:487: struct PathApplyInfo { Move this to anonymous namespace above. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:535: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) I was mentioning that I am not sure I understand this method. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:542: static RefPtr<TypeBuilder::Array<double> > buildArrayForQuadTypeBuilder(const FloatQuad& quad) I would have updated existing buildArrayForQuad with this behavior and would have changed the usage, but that would be done in a follow up patch. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:594: .setShape(buildArrayForShapeOutside(shapePath)) This I don't understand. shapePath is already an array. Shouldn't this be setShape(shapePath) ? https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:595: .setMarginShape(buildArrayForShapeOutside(marginShapePath)); ditto https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:588: drawPath(shapeInfo.marginShape, shapeMarginHighlightColor); But marginShape is just an array with a single element that is a path. How does that work? https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... Source/devtools/protocol.json:1930: { "name": "shape", "type": "array", "items": { "type": "any"}, "description": "Shape co-ordinate details" }, coordinate
I think most of the comments are due to conversion from JSONArray to TypeBuilder::Array::JSONValue that I am doing. As mentioned below I get a compilation error when passing JSON array value to TypeBuilder::Array, so thus creating an array. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:487: struct PathApplyInfo { On 2014/05/02 10:49:23, pfeldman wrote: > Move this to anonymous namespace above. Done. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:535: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) On 2014/05/02 10:49:23, pfeldman wrote: > I was mentioning that I am not sure I understand this method. I get following compilation error, if i pass JSONArray directly as parameter to setShape: "No known conversion for argument 1 from ‘WTF::RefPtr<WebCore::JSONArray>’ to ‘WTF::PassRefPtr<WebCore::TypeBuilder::Array<WebCore::JSONValue> >’" So this function is converting from JSONArray to TypeBuilder::Array::JSONValue. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:542: static RefPtr<TypeBuilder::Array<double> > buildArrayForQuadTypeBuilder(const FloatQuad& quad) On 2014/05/02 10:49:23, pfeldman wrote: > I would have updated existing buildArrayForQuad with this behavior and would > have changed the usage, but that would be done in a follow up patch. Ok, let me know will update accordingly. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:594: .setShape(buildArrayForShapeOutside(shapePath)) On 2014/05/02 10:49:23, pfeldman wrote: > This I don't understand. shapePath is already an array. Shouldn't this be > setShape(shapePath) ? Mentioned above reason for doing this. Idea is to convert from JSON array to TypeBuilder array. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlayPage.html:588: drawPath(shapeInfo.marginShape, shapeMarginHighlightColor); On 2014/05/02 10:49:23, pfeldman wrote: > But marginShape is just an array with a single element that is a path. How does > that work? It returns with same co-ordinates as shape. Path comprises of different commands such as move, line, curve, quadratic and that's used for drawing shape in drawPath. https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... Source/devtools/protocol.json:1930: { "name": "shape", "type": "array", "items": { "type": "any"}, "description": "Shape co-ordinate details" }, On 2014/05/02 10:49:23, pfeldman wrote: > coordinate Ok will update.
@pfeldman: Updated to use runtimecast instead of conversion done previously. I suppose it leave one comment in InspectorOverlayPage.html regarding how the array is used. I have given explanation how those array elements are used to draw in drawPath. Let me know if more explanation is needed for it. https://codereview.chromium.org/237313003/diff/250001/LayoutTests/inspector/e... File LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt (right): https://codereview.chromium.org/237313003/diff/250001/LayoutTests/inspector/e... LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt:1: [["M",88,8,"L",88,8,"C",132,8,168,44,168,88,"L",168,88,"C",168,132,132,168,88,168,"L",88,168,"C",44,168,8,132,8,88,"L",8,88,"C",8,44,44,8,88,8,"Z"]] On 2014/05/02 10:49:23, pfeldman wrote: > So shape is array of arrays. This does not sound right. Done. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:535: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) On 2014/05/02 10:49:23, pfeldman wrote: > I was mentioning that I am not sure I understand this method. Done. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:594: .setShape(buildArrayForShapeOutside(shapePath)) On 2014/05/02 10:49:23, pfeldman wrote: > This I don't understand. shapePath is already an array. Shouldn't this be > setShape(shapePath) ? Done. https://codereview.chromium.org/237313003/diff/250001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:595: .setMarginShape(buildArrayForShapeOutside(marginShapePath)); On 2014/05/02 10:49:23, pfeldman wrote: > ditto Done. https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/237313003/diff/250001/Source/devtools/protoco... Source/devtools/protocol.json:1930: { "name": "shape", "type": "array", "items": { "type": "any"}, "description": "Shape co-ordinate details" }, On 2014/05/02 10:49:23, pfeldman wrote: > coordinate Done.
I'd prefer to not use runtimeCast. See my comments on how this can be achieved. Otherwise, lgtm! https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:65: JSONArray* array; TypeBuilder::Array<JSONValue>* array; https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:498: info->array->pushString(command); info->array->addItem(JSONString::create(command)); https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:502: info->array->pushNumber(point.x()); info->array->addItem(JSONBasicValue::create(point.x())); https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:503: info->array->pushNumber(point.y()); info->array->addItem(JSONBasicValue::create(point.y())); https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:567: RefPtr<JSONArray> shapePath = JSONArray::create(); RefPtr<TypeBuilder::Array<JSONValue> > shapePath = TypeBuilder::Array<JSONValue>::create(); https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:568: RefPtr<JSONArray> marginShapePath = JSONArray::create(); RefPtr<TypeBuilder::Array<JSONValue> > marginShapePath = TypeBuilder::Array<JSONValue>::create();
Thanks Pavel. Updated as suggested. https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:65: JSONArray* array; On 2014/05/02 11:45:07, pfeldman wrote: > TypeBuilder::Array<JSONValue>* array; Done. https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:498: info->array->pushString(command); On 2014/05/02 11:45:07, pfeldman wrote: > info->array->addItem(JSONString::create(command)); Done. https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:502: info->array->pushNumber(point.x()); On 2014/05/02 11:45:07, pfeldman wrote: > info->array->addItem(JSONBasicValue::create(point.x())); Done. https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/I... Source/core/inspector/InspectorOverlay.cpp:503: info->array->pushNumber(point.y()); On 2014/05/02 11:45:07, pfeldman wrote: > info->array->addItem(JSONBasicValue::create(point.y())); Done.
LGTM for the core/rendering changes. https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:315: if (m_renderer.style()->isFlippedBlocksWritingMode()) Why isn't this if/else? https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:317: if (!m_renderer.style()->isHorizontalWritingMode()) Ditto.
Done. I was setting else condition values before checking if condition. But changed now to if/else condition. https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:315: if (m_renderer.style()->isFlippedBlocksWritingMode()) On 2014/05/02 20:41:02, leviw wrote: > Why isn't this if/else? Done. https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:317: if (!m_renderer.style()->isHorizontalWritingMode()) On 2014/05/02 20:41:02, leviw wrote: > Ditto. Done.
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/237313003/300001
https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:314: physicalBoundingBox.setX(physicalBoundingBox.x() + logicalLeftOffset()); Looks like leviw@ meant that only setY() should go under the conditional, since it could potentially get assigned a few times in a row. The setX() argument is always the same and the call can go right before the if/else's. https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:317: physicalBoundingBox.setX(physicalBoundingBox.x() + logicalLeftOffset()); This one will go away then... https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setX(physicalBoundingBox.x() + logicalLeftOffset()); ...as well as this one.
The CQ bit was unchecked by habib.virji@samsung.com
Thanks was not sure how to address the comment. Will do the change.
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/237313003/320001
Message was sent while issue was closed.
Change committed as 173379
Message was sent while issue was closed.
https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setY(physicalBoundingBox.y() + logicalTopOffset()); Great, that's exactly what I meant! I have a quick question, though. What is the relation between isFlippedBlocksWritingMode() and m_renderer.style()->isHorizontalWritingMode()? What if we encounter a situation where (isFlippedBlocksWritingMode() && isHorizontalWritingMode())? Then the Y set to (m_renderer.logicalHeight() - physicalBoundingBox.maxY()) will be overwritten by this (physicalBoundingBox.y() + logicalTopOffset()), right?
Message was sent while issue was closed.
https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setY(physicalBoundingBox.y() + logicalTopOffset()); Relation between isFlippedBlocksWritingMode() and isHorizontalWritingMode() --> It is to understand which direction to draw shapes. Since one shows either to show text in horizontal and other right/bottom mode. I suppose possibility of it together is not feasible. But rendering people might be in better option, I am too novice in this area. Then the Y set to (m_renderer.logicalHeight() - physicalBoundingBox.maxY()) will be overwritten by this (physicalBoundingBox.y() + logicalTopOffset()), right? --> Yes that's true and that's why previously setY was set before if condition. Though possibility of this looks rare. I will do further testing. Since this CL is landed, will try to upload new patch if encounter any issue to handle that scenario. Is that okay?
Message was sent while issue was closed.
https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/s... Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setY(physicalBoundingBox.y() + logicalTopOffset()); On 2014/05/06 12:25:41, Habib Virji wrote: >> Then the Y set to (m_renderer.logicalHeight() - physicalBoundingBox.maxY()) will >> be overwritten by this (physicalBoundingBox.y() + logicalTopOffset()), right? > --> Yes that's true and that's why previously setY was set before if condition. > Though possibility of this looks rare. I will do further testing. Since this CL > is landed, will try to upload new patch if encounter any issue to handle that > scenario. Is that okay? Sure. The best approach here is to try coming up with a test that fails and then fix the failure. |
