[webkit-reviews] review granted: [Bug 239674] Web Inspector: add UI for blocking requests : [Attachment 458183] Patch

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 239674: Web Inspector: add UI for blocking requests
https://bugs.webkit.org/show_bug.cgi?id=239674

Attachment 458183: Patch

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




--- Comment #4 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 458183
  --> https://bugs.webkit.org/attachment.cgi?id=458183
Patch

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

r=me neat stuff!

> Source/WebCore/ChangeLog:31
> +	   Add a `virtual` method so that `InspectorNetworkAgent` can log to
the (correct) console.

Just to make sure I understand correctly, by "correct" you mean the console
associated with Web Inspector where the Block override is configured?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1318
> +    auto resourceErrorType = ResourceError::Type::Null;
> +    switch (protocolResourceErrorType) {

Might make more sense for this switch statement to be its own static method
since it is just here to convert from a protocol value to a backend value now.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:527
> +		   requestURLRow.element.hidden = !isRequest || isBlock;

Is `|| isBlock` actually necessary? Shouldn't just making sure it isn't a
request cover any case where it would be block?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:529
> +		   methodRowElement.hidden = !isRequest || isBlock;

Ditto :527

>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideRequestContentVi
ew.js:75
> +	       message = document.createDocumentFragment();

Nit: I know we don't declare this here, but it still might be nice for this
assignment to be down just before :94 where it is used.