Bug 200683

Summary: Web Inspector: Sources: Give Origins their own icon in the Sources sidebar
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[IMAGE] Before - Light
none
[IMAGE] Before - Dark
none
[PATCH] Proposed Fix (Mono)
none
[IMAGE] After - Light (Mono)
none
[IMAGE] After - Dark (Mono)
none
[PATCH] Proposed Fix (Color)
hi: review+
[IMAGE] After - Light (Color)
none
[IMAGE] After - Dark (Color)
none
[PATCH] For Landing none

Description Joseph Pecoraro 2019-08-13 12:29:37 PDT
Give Origins their own icon in the Sources sidebar
Comment 1 Joseph Pecoraro 2019-08-13 12:29:58 PDT
Created attachment 376196 [details]
[IMAGE] Before - Light
Comment 2 Radar WebKit Bug Importer 2019-08-13 12:30:07 PDT
<rdar://problem/54269044>
Comment 3 Joseph Pecoraro 2019-08-13 12:30:10 PDT
Created attachment 376197 [details]
[IMAGE] Before - Dark
Comment 4 Joseph Pecoraro 2019-08-13 12:33:14 PDT
Created attachment 376198 [details]
[PATCH] Proposed Fix (Mono)
Comment 5 Joseph Pecoraro 2019-08-13 12:33:33 PDT
Created attachment 376199 [details]
[IMAGE] After - Light (Mono)
Comment 6 Joseph Pecoraro 2019-08-13 12:33:48 PDT
Created attachment 376200 [details]
[IMAGE] After - Dark (Mono)
Comment 7 Devin Rousso 2019-08-13 13:25:43 PDT
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});
```
Comment 8 Joseph Pecoraro 2019-08-13 14:11:34 PDT
Created attachment 376213 [details]
[PATCH] Proposed Fix (Color)
Comment 9 Joseph Pecoraro 2019-08-13 14:12:10 PDT
Created attachment 376214 [details]
[IMAGE] After - Light (Color)
Comment 10 Joseph Pecoraro 2019-08-13 14:12:27 PDT
Created attachment 376215 [details]
[IMAGE] After - Dark (Color)
Comment 11 Joseph Pecoraro 2019-08-19 22:27:48 PDT
Still reviewable. Might require a tiny rebaseline at this point.
Comment 12 Devin Rousso 2019-08-20 00:54:29 PDT
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).
Comment 13 Joseph Pecoraro 2019-08-20 03:25:31 PDT
Created attachment 376760 [details]
[PATCH] For Landing
Comment 14 WebKit Commit Bot 2019-08-20 12:18:06 PDT
Comment on attachment 376760 [details]
[PATCH] For Landing

Clearing flags on attachment: 376760

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