[webkit-reviews] review granted: [Bug 216138] Web Inspector: InvalidCharacterError: The string contains invalid characters. : [Attachment 408274] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 216138: Web Inspector: InvalidCharacterError: The string contains invalid
characters.
https://bugs.webkit.org/show_bug.cgi?id=216138

Attachment 408274: Patch

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




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 408274
  --> https://bugs.webkit.org/attachment.cgi?id=408274
Patch

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

r=me with some comments :)

>>>>> Source/WebInspectorUI/ChangeLog:9
>>>>> +        `DetailsSection`, which could end up with illegal characters due
to the escaping done by
>>>> 
>>>> I'm confused.  What logic actually changed to not display an illegal
character?  It seems like all you're doing now is not calling `CSS.escape` for
the shown text, which I think would reintroduce Bug 151378.
>>>> 
>>>> Perhaps we could just fix this by adding some extra character to the
beginning of the string if it starts with a number right before we `CSS.escape`
and then remove it after?
>>>> ```
>>>>	 let startsWithNumber = /\d/.test(id[0]);
>>>>	 if (startsWithNumber)
>>>>	     id = "n" + id;
>>>>	 id = CSS.escape(id);
>>>>	 if (startsWithNumber)
>>>>	     it = it.substring(1);
>>>> ```
>>> 
>>> The difference between Bug 151378 and this is that in that bug the text was
being used in the interface. We are looking for a way to identify the pieces of
Web Inspectors DOM by adding the string as a class in `WI.DetailsSection`. This
is why I added the option to not `CSS.escape` the text instead of just removing
the escape completely. All existing code paths continue to escape the id and
classes, but we can pull the new `identifier` property for the situations in
which we don't want the escaped form.
>>> 
>>> Your fix would work as well, but for it we would still need to make sure we
don't apply it when we actually want to use the result in the UI, otherwise we
end up with the same problem you had in Bug 151378 where the selector displayed
in the UI does not properly match because it is missing the escape characters.
>> 
>> If we don't call `CSS.escape` though, I don't think we'd display a properly
escaped selector for something like `id="#foo"` (which should be displayed as
"#\\#foo"), which was the original issue in Bug 151378.
>> 
>> Is there a way that we could maybe instead find and un-escape any
displayable code points after `CSS.escape`?  `\31 ` is displayable as "a", so
it's not like we actually need to show it as an escape character.
> 
> Theoretically `String.prototype.normalize()` should be able to unescape the
string for us. However, I don't think it's necessary. In this patch the only
time we don't call `CSS.escape` is when we are getting the properties for use
as a class on our DOM when building the sidebar. Previous we passed the
`displayName` from `DOMNode` to both the title and identifier parameters of the
constructor for `WI.DetailsSection`. Now we only pass the `CSS.escape` version
for the title, which is what is actually displayed in the inspector. The
identifier is used internally for us to keep track of the Inspector's DOM. For
that case we could use a random number if we didn't want the elements to have
more meaningful classes when inspecting the web inspector.
> 
> I'm attaching a screenshot showing after the patch that we still display the
`CSS.escape` for of the ID as to not regress Bug 151378.

Oh!  I see.  This only affects the `identifier`, not the `title`.  Gotcha. 
I'll do an actual review then :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:775
> +    get identifier()

This is possibly confusing in that there's already an `id` property on
`WI.DOMNode`.  Perhaps we could call this `get unescapedSelector`?

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:780
> +	   const escaped = false;

Style: it's usually fine to not have `const` variables if the function being
called is in the same file, but I'm also a fan of this so no problem :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1052
> +    _idSelector(escaped = true)

NIT: this name is a bit ambiguous, perhaps `shouldEscape`?

NIT: I'd also not have a default parameter for this (at least not one that's
truthy) so that someone reading `this._idSelector()` can know at the callsite
whether or not the selector is escaped.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1070
> +    _classSelector(escaped = true) {

ditto (:1052)

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:357
> +	       const identifier = `${options.identifier ??
title}-event-listener-section`

Style: we only use `const` for values that don't change between invocations of
Web Inspector, so this should be `let` as both `options.identifier` and/or
`title` can change

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:412
> +		   let section = createEventListenerSection(title,
eventListenersForTarget, {hideTarget: true, identifier: identifier});

Style: you can just do `{hideTarget: true, identifier}` when the key and value
are the same :)