[webkit-reviews] review granted: [Bug 198855] Web Inspector: Debugger: support pattern blackboxing : [Attachment 379605] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 198855: Web Inspector: Debugger: support pattern blackboxing
https://bugs.webkit.org/show_bug.cgi?id=198855

Attachment 379605: Patch

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




--- Comment #17 from Timothy Hatcher <timothy at apple.com> ---
Comment on attachment 379605
  --> https://bugs.webkit.org/attachment.cgi?id=379605
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:168
> +	       const shouldBlackbox = true;
> +	       {
> +		   const caseSensitive = true;

Nit: This style bothers me more than it likely should… but I'd add an empty
line after const shouldBlackbox.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:173
> +	       }
> +	       {
> +		   const isRegex = true;

Ditto. I would at least ad an empty line between these blocks (for my sanity.)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:447
> +	   console.assert(shouldBlackbox !==
((this.blackboxDataForSourceCode(sourceCode) || {}).type ===
DebuggerManager.BlackboxType.URL));

Nit: Group the assert with the asserts before and put the blank line below it
instead?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:471
> +	       let data = {
> +		   url: regex.source,
> +		   caseSensitive: !regex.ignoreCase,
> +	       };

Nit: Blank lines before and after..

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:478
> +	   }
> +	   this._blackboxedPatternsSetting.save();

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Images/Hide.svg:27
> +	   <symbol id="hide">

"hide" is an odd name, and could be confused. Maybe "eye"?

> Source/WebInspectorUI/UserInterface/Images/Hide.svg:36
> +    <svg id="currentColor"><use xlink:href="#hide"/></svg>
> +    <svg id="white"><use xlink:href="#hide"/></svg>
> +    <svg id="black"><use xlink:href="#hide"/></svg>

Clever use of vars, symbols and currentColor! Awesome that all works.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:129
> +	   });
> +	   this._blackboxPatternCodeMirrorMap.set(regex, urlCodeMirror);

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:176
> +	   };
> +	   urlCodeMirror.addKeyMap({

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:180
> +	   });
> +	   urlCodeMirror.on("blur", update);

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:183
> +	   if (WI.DebuggerManager.supportsBlackboxingScripts()) {
> +	       this._blackboxSettingsView = new WI.BlackboxSettingsView;
> +	       this.addSettingsView(this._blackboxSettingsView);
> +	   }

Nit: Blank lines before and after.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:226
> +	       }
> +	       console.assert(this._toggleBlackboxedImageElement.title);

Nit: Blank line between.