[webkit-reviews] review canceled: [Bug 177896] Web Inspector: Network Tab - Headers Detail View : [Attachment 322973] [PATCH] Proposed Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 6 11:15:29 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 177896: Web Inspector: Network Tab - Headers Detail View
https://bugs.webkit.org/show_bug.cgi?id=177896

Attachment 322973: [PATCH] Proposed Fix

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




--- Comment #24 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 322973
  --> https://bugs.webkit.org/attachment.cgi?id=322973
[PATCH] Proposed Fix

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

Thanks for the comments. New patch in a sec.

>> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:178
>>  FAIL: scheme should be: 'http'
> 
> Sucks that we have all these FAILs :(

Yep. One day we should move from `parseURL()` to `new URL(...)`.

>> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:41
>> +	padding: 2px 0 2px 7px;
> 
> To provide RTL support, you might want to split this into two properties.
> 
>     padding: 2px 0;
>     -webkit-padding-start: 7px;

Ahh, this is the first task where I forgot to do RTL testing! Thanks for the
pointers, I'll fix up RTL support.

>> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:45
>> +	color: var(--console-secondary-text-color) !important;
> 
> Is the "!important" absolutely necessary?

Nope, but I want it. Anyone who has marked a section as incomplete should
expect it.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:30
>> +	    super(null);
> 
> NIT: add a const variable.
> 
>     const representedObject = null;
>     super(representedObject);

I don't think we want to do this everywhere. Here it would just be noise.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:50
>> +	initialLayout()
> 
> Add call to `super.initialLayout();`.

I actually haven't seen other places doing this for initialLayout or layout. I
was hoping that would be the plan of record for View layout code. Otherwise
we'd have ~4 wasted lines in every view that ultimately does nothing.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:160
>> +	    let source =
this._responseSourceDisplayString(this._resource.responseSource) || emDash;
> 
> NIT: instead of having the `emDash` here, would it be better to have it as
the `default` return value above (142) so that if other callers are added they
don't need to do the same?

I'm going to keep this as is. I'm using emDash here only because I want the
Summary Section to not be variable height (so always have the same number of
rows). Other users may decide to ignore an empty value and should check against
empty / null and not emDash (or check the responseSource directly).


More information about the webkit-reviews mailing list