[webkit-reviews] review denied: [Bug 138810] Web Inspector: New CSS Rules should go into a new Stylesheet Resource that can be viewed/edited/saved : [Attachment 310458] Patch

Fri May 19 13:47:54 PDT 2017

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 138810: Web Inspector: New CSS Rules should go into a new Stylesheet
Resource that can be viewed/edited/saved
https://bugs.webkit.org/show_bug.cgi?id=138810

Attachment 310458: Patch

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

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

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

After applying I found some issues.

1. Styles sidebar shows a URL for the rule. It should show "Web Inspector".

r=me for this because it is a regression that makes a lot of sense.

2. Styles Sidebar doesn't immediately show changes to StyleSheet after
switching tabs

Steps to Reproduce:
1. Inspect this page
2. Show Elements Tab
3. Select <body>
4. Show Styles Rules sidebar
5. Click the (+) to add a new rule
6. Switch to the Resources tab
7. Modify the Inspector Style Sheet rule for "body {}"
8. Switch back to the Elements tab
  => Rule still has cursor and its empty. Clicking out suddenly loads the
content.

I think we will want to improve this back and forth. It might happen regardless
of the Inspector Style Sheet, but its something we should look into fixing. I
don't think we always want to blur the sidebar when switching tabs (maybe thats
what we end up on) but certain if the content changes out from under it we
should consider something here.

3. Uncaught exception!

[Error] Unhandled Promise Rejection: TypeError:
representedObject.isInspectorStyleSheet is not a function. (In
'representedObject.isInspectorStyleSheet()',
'representedObject.isInspectorStyleSheet' is undefined)
	fetchedStyleSheetContent (CSSStyleManager.js:527)
	fetchedStyleSheetContent
	promiseReactionJob

---

Other thoughts. I think we'll want a (+) button at the bottom of resources to
create an inspector stylesheet if one doesn't exist for the current page.
Because this is really super useful to just have these scratchpad stylesheets
without having to hunt for how to create it (creating a new rule in the styles
sidebar is obtuse).

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:170
> -    preferredInspectorStyleSheetForFrame(frame, callback)
> +    preferredInspectorStyleSheetForFrame(frame, callback,
dontCreateIfMissing)

Style: Don't use dont and Never use never. Use `doNot` and
notInTheForseeableFuture.

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:404
> -    _lookupStyleSheetForResource(resource, callback)
> +    _lookupStyleSheetForSourceCode(sourceCode, callback)
>      {
> -	   this._lookupStyleSheet(resource.parentFrame, resource.url,
callback);
> +	   this._lookupStyleSheet(sourceCode.parentFrame, sourceCode.url,
callback);
>      }

WebInspector.SourceCode doesn't have a `parentFrame` accessor, but Resource
does. This is starting to get hairy.

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:467
>	   // Ignore if it isn't a CSS stylesheet.
> -	   if (resource.type !== WebInspector.Resource.Type.Stylesheet ||
resource.syntheticMIMEType !== "text/css")
> +	   if (!(sourceCode instanceof WebInspector.CSSStyleSheet) &&
(sourceCode.type !== WebInspector.Resource.Type.Stylesheet ||
sourceCode.syntheticMIMEType !== "text/css"))
>	       return;

I think this will get called if Scripts get edited. So you may want a helper
which will make everything read easier and more correct:

    _isStyleSheet(sourceCode)
    {
	if (sourceCode instanceof WebInspector.CSSStyleSheet)
	    return true;
	if (sourceCode instanceof WebInspector.Resource)
	    return sourceCode.type === WebInspector.Resource.Type.Stylesheet &&
sourceCode.syntheticMIMEType === "text/css";
	return true;
    }

    ...

    if (!this._isStyleSheet(sourceCode))
	return;

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:528
> +	       if (representedObject.isInspectorStyleSheet())
> +		  
representedObject.dispatchEventToListeners(WebInspector.SourceCode.Event.Conten
tDidChange);

Is there any reason for this conditional? Can't we do this always?

Also, its possible that representedObject here is no longer a CSSStyleSheet
given it might change to a Resource above on line 508.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleSheetTreeElement.js:44
> +	   this._styleSheet = styleSheet;
> +    }
> +
> +    // Public
> +
> +    get styleSheet() { return this._styleSheet; }

You can drop the member and just use this.representedObject here.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:278
> +    _shouldBeReadOnly()

Having seen this now, the opposite name makes more sense. _shouldBeEditable.