Bug 200683 – Web Inspector: Sources: Give Origins their own icon in the Sources sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Give Origins their own icon in the Sources sidebar

[Joseph Pecoraro]

Created attachment 376196 [details]
[IMAGE] Before - Light

[Radar WebKit Bug Importer]

<rdar://problem/54269044>

[Joseph Pecoraro]

Created attachment 376197 [details]
[IMAGE] Before - Dark

[Joseph Pecoraro]

Created attachment 376198 [details]
[PATCH] Proposed Fix (Mono)

[Joseph Pecoraro]

Created attachment 376199 [details]
[IMAGE] After - Light (Mono)

[Joseph Pecoraro]

Created attachment 376200 [details]
[IMAGE] After - Dark (Mono)

[Devin Rousso]

Comment on attachment 376198 [details]
[PATCH] Proposed Fix (Mono)

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

> Source/WebInspectorUI/UserInterface/Images/Origin.svg:35
> +    	<g id="globe" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">

Why do we need `stroke="none" stroke-width="1" fill="none"`?

> Source/WebInspectorUI/UserInterface/Views/OriginTreeElement.js:28
> +    constructor(title, representedObject, isMainFrame, options)

I'd add `isMainFrame` to `options`, and make `options = {}`.

> Source/WebInspectorUI/UserInterface/Views/OriginTreeElement.js:34
> +        super(classNames, title, subtitle, representedObject, options);

Should we assume that this `hasChildren`, just like `WI.FolderTreeElement`?

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:603
> +            (treeElement) => treeElement instanceof WI.OriginTreeElement,

I think this should be put above `WI.FrameTreeElement`, as it's somewhat more important/unique (e.g. an origin could contain multiple frames).

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

I'd inline the `options`:
```
    this._mainFrameTreeElement = new WI.OriginTreeElement(mainFrame.securityOrigin, mainFrame, {isMainFrame: true, hasChildren: true});
```

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

Ditto (:648).
```
    originTreeElement = new WI.OriginTreeElement(origin, origin === resource.parentFrame.securityOrigin ? resource.parentFrame : null, {hasChildren: true});
```

[Joseph Pecoraro]

Created attachment 376213 [details]
[PATCH] Proposed Fix (Color)

[Joseph Pecoraro]

Created attachment 376214 [details]
[IMAGE] After - Light (Color)

[Joseph Pecoraro]

Created attachment 376215 [details]
[IMAGE] After - Dark (Color)

[Joseph Pecoraro]

Still reviewable. Might require a tiny rebaseline at this point.

[Devin Rousso]

Comment on attachment 376213 [details]
[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).

[Joseph Pecoraro]

Created attachment 376760 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 376760 [details]
[PATCH] For Landing

Clearing flags on attachment: 376760

Committed r248912: <https://trac.webkit.org/changeset/248912>