[webkit-reviews] review granted: [Bug 217032] Web Inspector: add UI for request interception : [Attachment 413136] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 217032: Web Inspector: add UI for request interception
https://bugs.webkit.org/show_bug.cgi?id=217032

Attachment 413136: Patch

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




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

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

r=me with some wholesome comments, great work!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1031
> +localizedStrings["Redirect"] = "Redirect";

Most of these string additions need a better UIString description.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1083
> +/* Text indicating that the local override will skip all network activity
and instead immediately serve the response. */

I bet this is going to be a heavy lift to localize. Thanks for adding more
context!

> Source/WebInspectorUI/UserInterface/Base/ReferencePage.js:30
> +    LocalOverrides: "local-overrides",

Nice.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:214
> +    get localResourceOverrides() { return this._localResourceOverrides; }

This should return a copy to retain the same behavior.

Is there a more modern way to do this besides Array.prototype.splice()?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:382
> +	  
console.assert(!this._localResourceOverrides.includes(localResourceOverride),
"Already had an existing local resource override.");

Please update the assert message ("local override already added"). It's now
possible for >1 override per resource so the current message no longer
describes an error condition.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:417
> +	   const rankFunctions = [

A short comment would be helpful for the uninitiated reader.

"Order local resource overrides that match the URL using multiple ranking
functions.
An override that matches an earlier ranking function will be ordered before an
override
that matches a later ranking function. Overrides with the same rank are in
insertion order."

Related question: is it intended for local resource overrides to maintain
insertion order when finding a match? That's what I would expect intuitively as
a user. I'm not sure that the code currently guarantees this, as overrides with
equal rank may not necessarily retain their insertion order during the sorting
algorithm.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:428
> +		   return aRank - bRank;

If aRank == bRank, then sort by insertion order. I don't think the order is
saved anywhere in this patch.

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:85
> +	   console.assert(false, type);

Nit: 'Unexpected type:', type

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:163
> +	       displayName = WI.UIString("%s (Case
Insensitive)").format(displayName);

This UIString needs more context ("label for a case-insensitive URL match
pattern in a local resource override").

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:184
> +	   if (localResourceOverrideOrSerializedData === this)

Huh?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:188
> +	   let shouldBeOverridden = resource.isLoading() &&
WI.networkManager.localResourceOverridesForURL(resource.url).some((localResourc
eOverride) => !localResourceOverride.disabled);

Nice.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:102
> +	       if (representedObject.type ===
WI.LocalResourceOverride.InterceptType.Request)

I'd prefer a switch since this is an enum, but this works too.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:87
> +			   initiatorHint:
WI.TabBrowser.TabNavigationInitiator.ContextMenu,

Nice, thanks for adding this.

>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.js:45
> +	   labelElement.textContent = this._localResourceOverride.type ===
WI.LocalResourceOverride.InterceptType.Request ? WI.UIString("Request
Override") : WI.UIString("Response Override");

Might want to add more UIString context. Where is this View in the UI?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:68
> +.popover .local-resource-override-popover-content:not(.request) .editor.url
{

Nit: it would be clearer to use .response instead of :not(.request).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:60
> +	       isCaseSensitive: !this._isCaseSensitiveCheckbox ||
this._isCaseSensitiveCheckbox.checked,

This looks wrong without reading the full context. In what case is
this._isCaseSensitiveCheckbox not assigned?

It would be nicer to use this.supportsCaseSensitive or something to represent
if this is user-modifiable for a given local override. I had to go look up all
the assignments to understand that this._isCaseSensitiveCheckbox is indeed tied
to a backend feature check.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:96
> +	       // NOTE: We can allow an empty mimeType / statusCode /
statusText to pass

I prefer to call this inherits/inherited fields to "pass through".

EDIT: Ah this is just code moving around. Nevermind.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:97
> +	       // network values through, but lets require them for overrides
so that

Nit: lets

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:105
> +	       if (isNaN(data.responseStatusCode))

This could be a followup, but it would be nice to have some visual feedback if
the inputted values are not valid (HTTP 666 for example).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:210
> +

Nit: extra newlines

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:281
> +	       let methodLabelElement =
methodHeaderElement.appendChild(document.createElement("label"));

Ditto (:210)

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:319
> +	   let statusCodeRow = createRow(WI.UIString("Status"), "status",
valueData.statusCode || "", placeholderData.statusCode);

This UIString needs some love. It's not obvious that this is a label for an
HTTP status code in the local overrides popover. I'd add a new string with a
different key.

>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideRequestContentVi
ew.js:56
> +	   saveData.suggestedName = WI.UIString("%s Request
Data").format(this.representedObject.url) + ".txt";

Needs a better UIString indicating that this is part of a suggested file name.