Bug 151594 – Web Inspector: support runtime registration of tab type associations

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

[Blaze Burg]

I want to add tabs that only exist in engineering builds. This would be done by putting the relevant models and content views in the Debug/ directory and registering it inside Bootstrap.js. But, the list of tab types is hardcoded. We should make it dynamic.

[Radar WebKit Bug Importer]

<rdar://problem/23659417>

[Blaze Burg]

Created attachment 266146 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 266146 [details]
Proposed Fix

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

Looks good. r=me

> Source/WebInspectorUI/ChangeLog:10
> +        for debugging purposes. Though the relevant models and views can be
> +        put Debug/ directory to exclude from production builds, there's no

Grammar: "can be put Debug/" needs refinement.

> Source/WebInspectorUI/UserInterface/Base/Main.js:444
> +    return new Set(this._knownTabClassesByType.values());

Maybe this should just return the values and not a set?

> Source/WebInspectorUI/UserInterface/Views/ConsoleTabContentView.js:38
> +    static tabInfo()

Style: We've been putting "static" functions inside a "// Static" block. But I've also considered just dropping that block and putting it above the "// Public" block like the constructor.

> Source/WebInspectorUI/UserInterface/Views/NewTabContentView.js:137
> +        allowedTabClasses.sort(function(a, b) {
> +            return a.tabInfo().title.localeCompare(b.tabInfo().title);
> +        });

Arrow function? foo.sort((a, b) => a.tabInfo().title.localeCompare(b.tabInfo().title))

[Blaze Burg]

(In reply to comment #3)
> Comment on attachment 266146 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266146&action=review
> 
> Looks good. r=me
> 
> > Source/WebInspectorUI/ChangeLog:10
> > +        for debugging purposes. Though the relevant models and views can be
> > +        put Debug/ directory to exclude from production builds, there's no
> 
> Grammar: "can be put Debug/" needs refinement.
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:444
> > +    return new Set(this._knownTabClassesByType.values());
> 
> Maybe this should just return the values and not a set?

I want to expose the fact that there are no duplicates as part of the API.

> > Source/WebInspectorUI/UserInterface/Views/ConsoleTabContentView.js:38
> > +    static tabInfo()
> 
> Style: We've been putting "static" functions inside a "// Static" block. But
> I've also considered just dropping that block and putting it above the "//
> Public" block like the constructor.

The block was more useful when we couldn't use the static keyword. I will move this up.

> 
> > Source/WebInspectorUI/UserInterface/Views/NewTabContentView.js:137
> > +        allowedTabClasses.sort(function(a, b) {
> > +            return a.tabInfo().title.localeCompare(b.tabInfo().title);
> > +        });
> 
> Arrow function? foo.sort((a, b) =>
> a.tabInfo().title.localeCompare(b.tabInfo().title))

[Blaze Burg]

Committed r193432: <http://trac.webkit.org/changeset/193432>