Bug 121719 - Web Inspector: [CSS Regions] Display CSS Regions chain when highlighting a CSS Region node
Summary: Web Inspector: [CSS Regions] Display CSS Regions chain when highlighting a CS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-20 14:38 PDT by Alexandru Chiculita
Modified: 2013-09-20 17:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch V1 (26.43 KB, patch)
2013-09-20 14:56 PDT, Alexandru Chiculita
joepeck: review+
Details | Formatted Diff | Diff
Screenshot (351.86 KB, image/jpeg)
2013-09-20 14:57 PDT, Alexandru Chiculita
no flags Details
Patch V2 (26.37 KB, patch)
2013-09-20 17:00 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V3 (26.37 KB, patch)
2013-09-20 17:03 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2013-09-20 14:38:15 PDT
Add code in the InspectorOverlay to highlight the chain of CSS Regions.
Comment 1 Radar WebKit Bug Importer 2013-09-20 14:38:53 PDT
<rdar://problem/15044651>
Comment 2 Alexandru Chiculita 2013-09-20 14:56:06 PDT
Created attachment 212222 [details]
Patch V1
Comment 3 Alexandru Chiculita 2013-09-20 14:57:13 PDT
Created attachment 212223 [details]
Screenshot
Comment 4 Joseph Pecoraro 2013-09-20 15:46:07 PDT
Comment on attachment 212222 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=212222&action=review

r=me! A few optional nits, feel free to cq+.

> Source/WebCore/ChangeLog:11
> +        When a CSS Region node is highlighted through the WebInspector, it will also lookup all the regions
> +        part of the same flow and inject enough information into InspectorOverlayPage.js to get the other

Nit: "all the regions part of the same flow" => "all the regions that are part of the same flow"

> Source/WebCore/inspector/InspectorOverlay.cpp:399
> +static void buildObjectForCSSRegionsHighlight(Node* node, InspectorObject* nodeObject)

Nit: I think naming "nodeObject" => "highlightNodeObject" would be clearer.

> Source/WebCore/inspector/InspectorOverlayPage.js:428
> +    }
> +
> +    for (var i = 0; i < regions.length; ++i) {
> +        var region = regions[i];

Nit: These top two loops can be combined without losing clarity. These lines could just be removed.

> Source/WebCore/testing/Internals.cpp:746
> +    RefPtr<InspectorObject> object = document->page()->inspectorController()->buildObjectForHighlightedNode();
> +    return object ? object->toJSONString() : String();

This JSON stringify/parse seems unnecessary. It seems the IDL for this could return a nullable object here instead of a string <http://www.w3.org/TR/WebIDL/#idl-object>:

    [RaisesException] object? inspectorHighlightObject(Document document);

However, having said that, I don't know what syntax would need to be here. It might even need to be Custom in that case. So I think what you have is fine.
Comment 5 Alexandru Chiculita 2013-09-20 17:00:33 PDT
Created attachment 212244 [details]
Patch V2
Comment 6 Alexandru Chiculita 2013-09-20 17:03:40 PDT
Created attachment 212247 [details]
Patch V3

Added reviewer in Changelogs this time.
Comment 7 WebKit Commit Bot 2013-09-20 17:34:04 PDT
Comment on attachment 212247 [details]
Patch V3

Clearing flags on attachment: 212247

Committed r156215: <http://trac.webkit.org/changeset/156215>
Comment 8 WebKit Commit Bot 2013-09-20 17:34:06 PDT
All reviewed patches have been landed.  Closing bug.