[webkit-reviews] review granted: [Bug 200683] Web Inspector: Sources: Give Origins their own icon in the Sources sidebar : [Attachment 376213] [PATCH] Proposed Fix (Color)

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 200683: Web Inspector: Sources: Give Origins their own icon in the Sources
sidebar
https://bugs.webkit.org/show_bug.cgi?id=200683

Attachment 376213: [PATCH] Proposed Fix (Color)

https://bugs.webkit.org/attachment.cgi?id=376213&action=review




--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 376213
  --> https://bugs.webkit.org/attachment.cgi?id=376213
[PATCH] Proposed Fix (Color)

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

r=me, nice!

> Source/WebInspectorUI/ChangeLog:15
> +	   (.origin-icon.main .icon):

This doesn't appear to exist in this patch.

> Source/WebInspectorUI/UserInterface/Images/Origin.svg:5
> +	   <radialGradient id="gradient" cx="58.60%" cy="27.74%" fx="58.60%"
fy="27.74%" r="57.50%" gradientTransform="translate(0.59, 0.28), rotate(98.61),
scale(1, 0.98), translate(-0.59, -0.28)">

NIT: you could remove the trailing `0` from `58.6%`.

> Source/WebInspectorUI/UserInterface/Images/Origin.svg:7
> +	       <stop stop-color="#A8ABB6" offset="0%"/>
> +	       <stop stop-color="#35374C" offset="100%"/>

We normally use `rgb()` values for our *.svg images.

> Source/WebInspectorUI/UserInterface/Images/Origin.svg:12
> +    <circle fill="none" stroke="#585A63" stroke-width="3" cx="31.9" cy="32"
r="28.9"/>

Ditto (6).

> Source/WebInspectorUI/UserInterface/Main.html:532
> +    <script src="Views/OriginTreeElement.js"></script>

Does this need to be this far "up" in the list, or can it be put in the more
general grouping further below?

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:648
> +	       this._mainFrameTreeElement = new
WI.OriginTreeElement(mainFrame.securityOrigin, mainFrame, {hasChildren: true});

This will need to be rebased.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:724
> +		       originTreeElement = new WI.OriginTreeElement(origin,
origin === resource.parentFrame.securityOrigin ? resource.parentFrame : null,
{hasChildren: true});

Ditto (648).