Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(128)

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.

Description

CSS 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+392 lines, -7 lines) Patch
M LayoutTests/http/tests/inspector/elements-test.js View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/inspector/elements/highlight-css-shapes-outside.html View 1 2 3 4 5 6 7 8 1 chunk +91 lines, -0 lines 0 comments Download
A LayoutTests/inspector/elements/highlight-css-shapes-outside-expected.txt View 1 2 3 4 5 6 10 11 12 13 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/inspector/InspectorDOMAgent.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/inspector/InspectorOverlay.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +3 lines, -0 lines 0 comments Download
M Source/core/inspector/InspectorOverlay.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +112 lines, -2 lines 0 comments Download
M Source/core/inspector/InspectorOverlayPage.html View 1 2 3 4 5 6 7 8 9 3 chunks +53 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/BoxShape.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/shapes/BoxShape.cpp View 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/PolygonShape.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/shapes/PolygonShape.cpp View 1 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/RasterShape.h View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/RasterShape.cpp View 1 2 3 4 1 chunk +18 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/RectangleShape.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/shapes/RectangleShape.cpp View 1 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/Shape.h View 3 chunks +6 lines, -0 lines 0 comments Download
M Source/core/rendering/shapes/ShapeOutsideInfo.h View 1 2 3 4 5 1 chunk +7 lines, -3 lines 0 comments Download
M Source/core/rendering/shapes/ShapeOutsideInfo.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +35 lines, -0 lines 3 comments Download
M Source/devtools/Inspector-1.1.json View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M Source/devtools/protocol.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +13 lines, -1 line 0 comments Download

Messages

Total messages: 52 (0 generated)
jochen (gone - plz use gerrit)
+pfeldman
6 years, 8 months ago (2014-04-15 11:38:22 UTC) #1
pfeldman
https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/InspectorOverlay.cpp#newcode525 Source/core/inspector/InspectorOverlay.cpp:525: PathApplyInfo* pathApplyInfo = static_cast<PathApplyInfo*>(info); Please don't reinterpret, call it ...
6 years, 8 months ago (2014-04-15 11:59:08 UTC) #2
rwlbuis
Looks quite good to me but found a few issues still. https://codereview.chromium.org/237313003/diff/1/Source/core/rendering/shapes/PolygonShape.cpp File Source/core/rendering/shapes/PolygonShape.cpp (right): ...
6 years, 8 months ago (2014-04-15 15:48:21 UTC) #3
Habib Virji
@pfeldman and @rwlbuis: updated as per review comments and added tests. Only thing did not ...
6 years, 8 months ago (2014-04-17 15:43:47 UTC) #4
rwlbuis
On 2014/04/17 15:43:47, Habib Virji wrote: > @pfeldman and @rwlbuis: updated as per review comments ...
6 years, 8 months ago (2014-04-17 16:03:51 UTC) #5
apavlov
https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/1/Source/core/inspector/InspectorOverlay.cpp#newcode525 Source/core/inspector/InspectorOverlay.cpp:525: PathApplyInfo* pathApplyInfo = static_cast<PathApplyInfo*>(info); On 2014/04/17 15:43:47, Habib Virji ...
6 years, 8 months ago (2014-04-17 16:11:31 UTC) #6
Habib Virji
On 2014/04/17 16:03:51, rwlbuis wrote: > On 2014/04/17 15:43:47, Habib Virji wrote: > > @pfeldman ...
6 years, 8 months ago (2014-04-17 16:12:25 UTC) #7
apavlov
https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/shapes/RasterShape.cpp File Source/core/rendering/shapes/RasterShape.cpp (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/rendering/shapes/RasterShape.cpp#newcode128 Source/core/rendering/shapes/RasterShape.cpp:128: int endY = y + 1; On 2014/04/17 16:11:32, ...
6 years, 8 months ago (2014-04-17 16:12:26 UTC) #8
Habib Virji
@apavlov: thanks for review. Corrected the changes suggested https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/InspectorOverlayPage.html File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/40001/Source/core/inspector/InspectorOverlayPage.html#newcode349 Source/core/inspector/InspectorOverlayPage.html:349: while(commandsIndex ...
6 years, 8 months ago (2014-04-17 16:27:01 UTC) #9
apavlov
https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/InspectorOverlayPage.html File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/InspectorOverlayPage.html#newcode392 Source/core/inspector/InspectorOverlayPage.html:392: if (!shapeInfo.shape || !shapeInfo.marginShape) This is wrong: !(a || ...
6 years, 8 months ago (2014-04-17 16:39:11 UTC) #10
Habib Virji
Thanks for pointing out. corrected now. https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/InspectorOverlayPage.html File Source/core/inspector/InspectorOverlayPage.html (right): https://codereview.chromium.org/237313003/diff/60001/Source/core/inspector/InspectorOverlayPage.html#newcode392 Source/core/inspector/InspectorOverlayPage.html:392: if (!shapeInfo.shape || ...
6 years, 8 months ago (2014-04-17 16:43:15 UTC) #11
apavlov
On 2014/04/17 16:43:15, Habib Virji wrote: > Thanks for pointing out. Corrected Thanks for fixing ...
6 years, 8 months ago (2014-04-17 16:45:48 UTC) #12
Bear Travis
I'm excited to see this patch making progress. The code looks pretty solid to me, ...
6 years, 8 months ago (2014-04-17 20:26:39 UTC) #13
Habib Virji
Apologise for delay in submitting changes, we had holiday on Friday and Monday. Yesterday ran ...
6 years, 8 months ago (2014-04-23 16:28:32 UTC) #14
Bear Travis
FWIW LGTM with some small nits. You don't have to write assert-style tests unless you ...
6 years, 8 months ago (2014-04-23 20:46:05 UTC) #15
Habib Virji
Thanks Bear Travis. Addressed your comments. https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/inspector/elements-test.js File LayoutTests/http/tests/inspector/elements-test.js (right): https://codereview.chromium.org/237313003/diff/110001/LayoutTests/http/tests/inspector/elements-test.js#newcode758 LayoutTests/http/tests/inspector/elements-test.js:758: for(var commandsIndex = ...
6 years, 8 months ago (2014-04-24 09:53:20 UTC) #16
apavlov
Devtools part looks good. Pavel, do you have any reservations on the patch?
6 years, 8 months ago (2014-04-24 12:42:03 UTC) #17
rwlbuis
Looks pretty good, found some small stuff. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/InspectorController.h File Source/core/inspector/InspectorController.h (right): https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/InspectorController.h#newcode70 Source/core/inspector/InspectorController.h:70: class RenderBox; ...
6 years, 8 months ago (2014-04-24 15:17:26 UTC) #18
Habib Virji
Thanks Rob, addressed your comments. @pavel: let me know if anything more is needed. https://codereview.chromium.org/237313003/diff/130001/Source/core/inspector/InspectorController.h ...
6 years, 8 months ago (2014-04-24 17:12:21 UTC) #19
pfeldman
This looks great! The only question I have is test-related. Once it is addressed, we ...
6 years, 8 months ago (2014-04-25 12:31:01 UTC) #20
Habib Virji
Updated to use inspector protocol and made it part of getBoxModel. Kept it separated from ...
6 years, 7 months ago (2014-04-28 15:07:54 UTC) #21
pfeldman
https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/InspectorOverlay.cpp#newcode764 Source/core/inspector/InspectorOverlay.cpp:764: shapeOutsideStr = shapeOutsideInfo ? shapeOutsideInfo->toJSONString() : String(); You should ...
6 years, 7 months ago (2014-04-30 07:43:47 UTC) #22
Habib Virji
@pfeldman: Updated as suggested. https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/170001/Source/core/inspector/InspectorOverlay.cpp#newcode764 Source/core/inspector/InspectorOverlay.cpp:764: shapeOutsideStr = shapeOutsideInfo ? shapeOutsideInfo->toJSONString() ...
6 years, 7 months ago (2014-05-01 09:52:06 UTC) #23
pfeldman
https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp#newcode1381 Source/core/inspector/InspectorDOMAgent.cpp:1381: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) I'm not sure I ...
6 years, 7 months ago (2014-05-01 10:18:49 UTC) #24
Habib Virji
https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp#newcode1381 Source/core/inspector/InspectorDOMAgent.cpp:1381: static RefPtr<TypeBuilder::Array<JSONValue> > buildArrayForShapeOutside(PassRefPtr<JSONValue> value) On 2014/05/01 10:18:50, pfeldman ...
6 years, 7 months ago (2014-05-01 10:38:06 UTC) #25
Habib Virji
On 2014/05/01 10:18:49, pfeldman wrote: > https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp > File Source/core/inspector/InspectorDOMAgent.cpp (right): > > https://codereview.chromium.org/237313003/diff/190001/Source/core/inspector/InspectorDOMAgent.cpp#newcode1381 > ...
6 years, 7 months ago (2014-05-01 12:38:34 UTC) #26
pfeldman
> @pfeldman: Using ShapeOutsideInfo in inspectorOverlay requires converting > JSONValue to TypeBuilder::DOM::ShapeOutsideInfo and then for ...
6 years, 7 months ago (2014-05-01 13:19:07 UTC) #27
Habib Virji
"Oh, it is that JSONObjectBase won't let you cast to JSONObject..." --> Yes that's the ...
6 years, 7 months ago (2014-05-01 13:45:03 UTC) #28
pfeldman
> "Could you build ShapeOutsideInfo once and then send it over the protocol as > ...
6 years, 7 months ago (2014-05-01 13:56:27 UTC) #29
Habib Virji
" Could you elaborate on this one, I am not sure I completely follow." --> ...
6 years, 7 months ago (2014-05-01 14:18:01 UTC) #30
pfeldman
This looks much better. One final update and it is good to go! https://codereview.chromium.org/237313003/diff/210001/Source/core/inspector/InspectorOverlay.cpp File ...
6 years, 7 months ago (2014-05-02 07:21:54 UTC) #31
Habib Virji
Thanks pfeldman, updated as suggested. Particularly asObject convesion was really handy. Few extra checks to ...
6 years, 7 months ago (2014-05-02 09:39:22 UTC) #32
pfeldman
https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/230001/Source/core/inspector/InspectorOverlay.cpp#newcode559 Source/core/inspector/InspectorOverlay.cpp:559: if (!renderer || !renderer->isBox() || !toRenderBox(renderer)->shapeOutsideInfo()) // This check ...
6 years, 7 months ago (2014-05-02 10:12:06 UTC) #33
Habib Virji
Removed comments and JSONObject created. Only thing might be bounds, which I have kept as ...
6 years, 7 months ago (2014-05-02 10:33:07 UTC) #34
pfeldman
I made one more path over inspector-related core and there is one last thing I ...
6 years, 7 months ago (2014-05-02 10:49:22 UTC) #35
Habib Virji
I think most of the comments are due to conversion from JSONArray to TypeBuilder::Array::JSONValue that ...
6 years, 7 months ago (2014-05-02 11:11:27 UTC) #36
Habib Virji
@pfeldman: Updated to use runtimecast instead of conversion done previously. I suppose it leave one ...
6 years, 7 months ago (2014-05-02 11:38:51 UTC) #37
pfeldman
I'd prefer to not use runtimeCast. See my comments on how this can be achieved. ...
6 years, 7 months ago (2014-05-02 11:45:06 UTC) #38
Habib Virji
Thanks Pavel. Updated as suggested. https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/InspectorOverlay.cpp File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/237313003/diff/270001/Source/core/inspector/InspectorOverlay.cpp#newcode65 Source/core/inspector/InspectorOverlay.cpp:65: JSONArray* array; On 2014/05/02 ...
6 years, 7 months ago (2014-05-02 12:39:31 UTC) #39
leviw_travelin_and_unemployed
LGTM for the core/rendering changes. https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/shapes/ShapeOutsideInfo.cpp File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/270002/Source/core/rendering/shapes/ShapeOutsideInfo.cpp#newcode315 Source/core/rendering/shapes/ShapeOutsideInfo.cpp:315: if (m_renderer.style()->isFlippedBlocksWritingMode()) Why isn't ...
6 years, 7 months ago (2014-05-02 20:41:02 UTC) #40
Habib Virji
Done. I was setting else condition values before checking if condition. But changed now to ...
6 years, 7 months ago (2014-05-06 08:50:24 UTC) #41
Habib Virji
The CQ bit was checked by habib.virji@samsung.com
6 years, 7 months ago (2014-05-06 08:50:36 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/237313003/300001
6 years, 7 months ago (2014-05-06 08:50:47 UTC) #43
apavlov
https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/300001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp#newcode314 Source/core/rendering/shapes/ShapeOutsideInfo.cpp:314: physicalBoundingBox.setX(physicalBoundingBox.x() + logicalLeftOffset()); Looks like leviw@ meant that only ...
6 years, 7 months ago (2014-05-06 09:12:09 UTC) #44
Habib Virji
The CQ bit was unchecked by habib.virji@samsung.com
6 years, 7 months ago (2014-05-06 09:18:56 UTC) #45
Habib Virji
Thanks was not sure how to address the comment. Will do the change.
6 years, 7 months ago (2014-05-06 09:19:33 UTC) #46
Habib Virji
The CQ bit was checked by habib.virji@samsung.com
6 years, 7 months ago (2014-05-06 10:40:14 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/237313003/320001
6 years, 7 months ago (2014-05-06 10:40:27 UTC) #48
commit-bot: I haz the power
Change committed as 173379
6 years, 7 months ago (2014-05-06 11:47:32 UTC) #49
apavlov
https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp#newcode323 Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setY(physicalBoundingBox.y() + logicalTopOffset()); Great, that's exactly what I meant! ...
6 years, 7 months ago (2014-05-06 12:08:34 UTC) #50
Habib Virji
https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/237313003/diff/320001/Source/core/rendering/shapes/ShapeOutsideInfo.cpp#newcode323 Source/core/rendering/shapes/ShapeOutsideInfo.cpp:323: physicalBoundingBox.setY(physicalBoundingBox.y() + logicalTopOffset()); Relation between isFlippedBlocksWritingMode() and isHorizontalWritingMode() --> ...
6 years, 7 months ago (2014-05-06 12:25:40 UTC) #51
apavlov
6 years, 7 months ago (2014-05-06 12:30:28 UTC) #52
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.

Powered by Google App Engine
This is Rietveld 408576698