Bug 69351 – Web Inspector: make ResourceView(s) compile

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 109645 [details]
Patch

[Pavel Feldman]

Created attachment 109655 [details]
[Patch] More ResourceViews.

[Yury Semikhatsky]

Comment on attachment 109655 [details]
[Patch] More ResourceViews.

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

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:328
> +        if (typeof headersLength === "number") {

Extract all this block into a separate method and call it explicitly where needed instead of doing this odd check here.

[Pavel Feldman]

Comment on attachment 109655 [details]
[Patch] More ResourceViews.

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

>> Source/WebCore/inspector/front-end/ResourceHeadersView.js:328

> 
> Extract all this block into a separate method and call it explicitly where needed instead of doing this odd check here.

This method has 6 fields and 6 refresh methods that update corresponding fields. I would not want to split one of them into two parts for the following reasons:
- Call site might get confused on whether it should call both methods of only one
- This is a classical render(title, opt_subtitle). Not sure why call site should have two methods for this case
- Imagine title owning subtitle element, how would you allow title to position (define location of) subtitle element

[Pavel Feldman]

> Extract all this block into a separate method and call it explicitly where needed instead of doing this odd check here.

Does this mean that the change suggested in required in order to let this patch in?

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 109655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109655&action=review
> 
> >> Source/WebCore/inspector/front-end/ResourceHeadersView.js:328
> 
> > 
> > Extract all this block into a separate method and call it explicitly where needed instead of doing this odd check here.
> 
> This method has 6 fields and 6 refresh methods that update corresponding fields. I would not want to split one of them into two parts for the following reasons:
> - Call site might get confused on whether it should call both methods of only one
Method name should eliminate such confusion.

> - This is a classical render(title, opt_subtitle). Not sure why call site should have two methods for this case
Because it is a different than a public method on TreeElement or such in that it is a private method which is called from two places: at first with 3 params, at second 2 params and because of that you have to an id block to the method which could well be avoided.

> - Imagine title owning subtitle element, how would you allow title to position (define location of) subtitle element
Extract title creation into a separate method that would return a container where to insert the subtitle?

[Yury Semikhatsky]

(In reply to comment #5)
> > Extract all this block into a separate method and call it explicitly where needed instead of doing this odd check here.
> 
> Does this mean that the change suggested in required in order to let this patch in?

Although it would make the inspector code more clear, I wouldn't block this patch  if you have a principal disagreement on this change.

[Pavel Feldman]

Landed as http://svn.webkit.org/repository/webkit/trunk@96695