Summary: | Web Inspector: Sources: Give Origins their own icon in the Sources sidebar | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||
Component: | Web Inspector | Assignee: | 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
Joseph Pecoraro
2019-08-13 12:29:37 PDT
Created attachment 376196 [details]
[IMAGE] Before - Light
Created attachment 376197 [details]
[IMAGE] Before - Dark
Created attachment 376198 [details]
[PATCH] Proposed Fix (Mono)
Created attachment 376199 [details]
[IMAGE] After - Light (Mono)
Created attachment 376200 [details]
[IMAGE] After - Dark (Mono)
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}); ``` Created attachment 376213 [details]
[PATCH] Proposed Fix (Color)
Created attachment 376214 [details]
[IMAGE] After - Light (Color)
Created attachment 376215 [details]
[IMAGE] After - Dark (Color)
Still reviewable. Might require a tiny rebaseline at this point. 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). Created attachment 376760 [details]
[PATCH] For Landing
Comment on attachment 376760 [details] [PATCH] For Landing Clearing flags on attachment: 376760 Committed r248912: <https://trac.webkit.org/changeset/248912> |