Bug 147534 – Web Inspector: Convert remaining ContentViews to use ES6 classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

,

[Brian Burg]

Created attachment 258038 [details]
WIP

[Blaze Burg]

Created attachment 258227 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 258227 [details]
Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:40
> +        this._contentBrowser = contentBrowser; // Must call the super constructor before using `this`.

I'm not sure the comment is needed. It should be obvious, in the future if someone does this wrong as it will result in a runtime TDZ error. You've done the hard work here, and reordered the code appropriately.

> Source/WebInspectorUI/UserInterface/Views/ContentFlowDOMTreeContentView.js:35
> +        console.assert(contentFlow);

We should assert that this is instanceof something.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:43
> +    // Public

We've been going with a "// Static" section before a "// Public" section.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:35
> +    };

I do not believe this semicolon is allowed. I'd have expected a parse error, but maybe it is allowed! Either way, drop it.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:40
> +    static cookieMatchesResourceURL(cookie, resourceURL)

Style: "// Static" header before "// Public".

We can consider just dropping the "// Static" comments... I'm not sure they are valuable.

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:30
> +        console.assert(domTree);

Nit: instanceof

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:30
> +        console.assert(resource);

Nit: instanceof

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:31
> +        console.assert(styleClassName);

Nit: typeof === "string"

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:30
> +        super(script);

Nit: Could assert instanceof WebInspector.Script.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:576
> +        this._timelineViewMap.set(timeline, WebInspector.ContentView.createFromRepresentedObject(timeline, {timelineSidebarPanel: this._timelineSidebarPanel}));

I'm surprised there aren't more places where we called "new WebInspector.ContentView"!

[Blaze Burg]

Committed r187967: <http://trac.webkit.org/changeset/187967>