[webkit-reviews] review canceled: [Bug 110641] WebInspector: Switch hide element shortcut in ElementsPanel to use a selector : [Attachment 190613] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled
egraether at chromium.org's request for review:
Bug 110641: WebInspector: Switch hide element shortcut in ElementsPanel to use
a selector
https://bugs.webkit.org/show_bug.cgi?id=110641

Attachment 190613: Patch
https://bugs.webkit.org/attachment.cgi?id=190613&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190613&action=review


> Source/WebCore/ChangeLog:11
> +	   the document by creating a style tag in the documents head and in
the head of each

Could you test that the feature works for nested frames, too? Please see
LayoutTests/inspector/styles/resources/styles-source-lines-inline-iframe.html
for how to synchronize on iframe loading.

> Source/WebCore/inspector/front-end/DOMAgent.js:800
> +	   var classString = this.getAttribute("class") || "";

You could implement this using the same RuntimeAgent.evaluate() approach as
below if you could retrieve the target node in the evaluated code. Pavel can
advise on this. If that were possible, the code would be as simple as

var toggleClass = function(className, enable)
{
    var node = ...
    if (enable)
	node.classList.add(className);
    else
	node.classList.remove(className);
}

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:620
> +	* @param {function()=} userCallback

This signature should match what toggleClassName() accepts:

{function(?Protocol.Error)=}

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:624
> +	   var className = "web-inspector-hide-shortcut";

I would use some weird system-like prefix for these className and styleTagId,
perhaps "__" or similar, in order to avoid collisions with the user code as
much as possible.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:628
> +	   var injectStyleRuleCode = [

You can just declare "var injectStyleRuleCode = function() {...}" and then
evaluate something like
"(" + injectStyleRuleCode.toString() + ")()"
(take a look at how a similar thing is done in
DOMAgent.prototype._emulateTouchEventsChanged. That approach will barely work
for you, since it requires a page reload for the code to get injected).

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:629
> +	       "var style = document.head.querySelector('style#" + styleTagId +
"');",

Excellent

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:646
> +	   RuntimeAgent.evaluate(injectStyleRuleCode,
runtimeCallback.bind(this));

Are you sure it evaluates the script in every frame out there?

> LayoutTests/inspector/elements/hide-shortcut.html:137
> +Tests the hide shortcut, which toggles a class name on the node and sets
visibility: hidden on the node and it's ancestors.

We usually specify a link to the respective webkit bug after the description:

...and its ancestors. <a
href="https://bugs.webkit.org/show_bug.cgi?id=110641">Bug 110641</a>.