[webkit-reviews] review granted: [Bug 200425] Web Inspector: Sources: provide a way to create an arbitrary Inspector Style Sheet : [Attachment 375608] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200425: Web Inspector: Sources: provide a way to create an arbitrary
Inspector Style Sheet
https://bugs.webkit.org/show_bug.cgi?id=200425

Attachment 375608: Patch

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




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375608
  --> https://bugs.webkit.org/attachment.cgi?id=375608
Patch

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

r=me

I still think the (+) button should be in the left corner. Have you played with
that? Can we file a follow-up to experiment with that and see if it feels
better? That matches the rest of the system, and the consistently of knowing
"the bottom left corner will always have this button" is a conceptual win.

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:244
> +    if (options.allowDirectoryAsName && url !== "about:blank" &&
(!displayName || urlComponents.path.endsWith(displayName + "/")))

This makes me think we probably have others ("about:srcdoc" comes to mind).

Also `javascript:`, `mailto:`, `about:`, `blob:` and other common prefixes.

Most of what I've been doing recently is checking if the url starts with
"http:" to be more proactive then reactive. Does that make sense here?

We should really cover `WI.displayNameForURL` with tests.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1679
> +	       let addInspectorStyleSheetCheckbox = (parent, frame) => {

`parent` sounds like a node or tree element. Perhaps `menu` would be clearer.

> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:252
> +FAIL: origin should be: 'http://example.com'
> +    Expected: "http://example.com"
> +    Actual: null

Whats up with these failures?