[webkit-reviews] review denied: [Bug 168709] Web Inspector: Don't show the Search tab if it's open and matches the representedObject : [Attachment 302369] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 30 10:01:22 PDT 2017


Brian Burg <bburg at apple.com> has denied Devin Rousso <webkit at devinrousso.com>'s
request for review:
Bug 168709: Web Inspector: Don't show the Search tab if it's open and matches
the representedObject
https://bugs.webkit.org/show_bug.cgi?id=168709

Attachment 302369: Patch

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




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 302369
  --> https://bugs.webkit.org/attachment.cgi?id=302369
Patch

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

Let's see one more iteration on this patch. I think we can make the options
less distracting with my suggestions.

> Source/WebInspectorUI/UserInterface/Base/Main.js:813
> +    this.openURL(anchorElement.href, frame, Object.shallowMerge(options, {

I don't think this needs to be line wrapped. There is only one property.

> Source/WebInspectorUI/UserInterface/Base/Main.js:851
> +	       positionToReveal,

Ditto.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1406
> +	   this.showSourceCodeForFrame(frame.id, {

I prefer using `let options = { ... }` rather than line wrapping function
calls. I'd prefer we only wrap function call sites when passing a
callback/completion handler.

> Source/WebInspectorUI/UserInterface/Base/Main.js:-2321
> -	   anchor.lineNumber = lineNumber;

Line and column numbers do not appear to be optional so they should be a
positional argument. BTW, we should use WebInspector.SourceCodePosition in
place of line+col.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2337
> +	   dontFloat: true,

No line wrap.

>
Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:
70
> +	      
WebInspector.showOriginalOrFormattedSourceCodeLocation(breakpoint.sourceCodeLoc
ation, {

I would not wrap, or use a local.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:113
> +	       ignoreNetworkTab: true,

Ditto.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:252
> +		       ignoreNetworkTab: true,

Ditto.

>
Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1675
> +		   ignoreNetworkTab: true,

Ditto.

>
Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1687
> +		   ignoreNetworkTab: true,

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:707
> +	       columnNumber,

use WebInspector.SourceCodePosition

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:62
> +	       columnNunber: parseInt(columnNumber),

Typo: columnNunber

use WebInspector.SourceCodePosition instead.

>
Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:3
18
> +			   ignoreNetworkTab: true,

Are there any cases where we don't pass ignoreNetworkTab? Perhaps it would be
easier to negate the option to be allowsNetworkTab or usesNetworkTab. This
would draw more attention to the exceptional cases, which are currently hard to
see.


More information about the webkit-reviews mailing list