[DevTools] Adjust svg elements highlight to the root FrameView origin.

[DevTools] Adjust svg elements highlight to the root FrameView origin.

BUG=421404
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184366

[dgozman, 2014-10-16 10:45:49]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org

[dgozman, 2014-10-16 10:45:50]

Take a look please. I think this was regressed in
https://codereview.chromium.org/430463002/.

[caseq, 2014-10-16 11:35:00]

Can we have a test for this please?

[dgozman, 2014-10-17 17:06:47]

On 2014/10/16 11:35:00, caseq wrote:
> Can we have a test for this please?

I've tried, but we don't expose generic highlight shape through protocol, which
is required for SVG elements.

[caseq, 2014-10-20 15:46:02]

On 2014/10/17 17:06:47, dgozman wrote:
> On 2014/10/16 11:35:00, caseq wrote:
> > Can we have a test for this please?
> 
> I've tried, but we don't expose generic highlight shape through protocol,
which
> is required for SVG elements.

Let's add window.internals method?

[dgozman, 2014-10-21 16:10:40]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2014-10-21 16:10:41]

+pfeldman for Source/core/testing

Take another look please.

[dgozman, 2014-10-24 11:03:00]

ping

[pfeldman, 2014-10-24 13:30:00]

lgtm w/ nit

https://codereview.chromium.org/662603002/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/662603002/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:187: [RaisesException] DOMString
getInspectorHighlightJSON(Node node);
nuke the get prefix.

[caseq, 2014-10-24 13:31:45]

https://codereview.chromium.org/662603002/diff/20001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/elements-test.js (right):

https://codereview.chromium.org/662603002/diff/20001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/elements-test.js:780:
InspectorTest.evaluateInPage("window.internals.getInspectorHighlightJSON(" +
getDocument + ".getElementById(\"" + nodeId + "\"))", innerCallback);
For better readability, please either extract into a function and use
func.toString() here or define function in the inspected page.

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorOverlay.cpp:840: String
InspectorOverlay::getHighlightJSONForNode(Node* node)
Can we return JSONObject for better reuse? It's never too late to stringize it.

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorOverlay.cpp:842: HighlightConfig config;
So we end up with a set of path that are all the same color and hence impossible
to distinguish. Should we make the colors different to improve coverage?

[dgozman, 2014-10-24 14:45:21]

https://codereview.chromium.org/662603002/diff/20001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/elements-test.js (right):

https://codereview.chromium.org/662603002/diff/20001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/elements-test.js:780:
InspectorTest.evaluateInPage("window.internals.getInspectorHighlightJSON(" +
getDocument + ".getElementById(\"" + nodeId + "\"))", innerCallback);
On 2014/10/24 13:31:44, caseq wrote:
> For better readability, please either extract into a function and use
> func.toString() here or define function in the inspected page.

Done.

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorOverlay.cpp:840: String
InspectorOverlay::getHighlightJSONForNode(Node* node)
On 2014/10/24 13:31:44, caseq wrote:
> Can we return JSONObject for better reuse? It's never too late to stringize
it.

Done.

https://codereview.chromium.org/662603002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorOverlay.cpp:842: HighlightConfig config;
On 2014/10/24 13:31:44, caseq wrote:
> So we end up with a set of path that are all the same color and hence
impossible
> to distinguish. Should we make the colors different to improve coverage?

Nice idea! Done.

https://codereview.chromium.org/662603002/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/662603002/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:187: [RaisesException] DOMString
getInspectorHighlightJSON(Node node);
On 2014/10/24 13:30:00, pfeldman wrote:
> nuke the get prefix.

Done.

[dgozman, 2014-10-24 14:47:37]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-10-24 14:48:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/662603002/40001

[commit-bot: I haz the power, 2014-10-24 16:01:19]

Committed patchset #3 (id:40001) as 184366