Bug 86367 – Web Inspector: Use CSS columns feature for HelpScreen contents.

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

Right-docked inspector should show shortcuts/settings in one column.
Bottom-docked inspector should use horizontal space more effectively (more than 2 columns in shortcuts/settings).

Please see attached screenshots.

Created attachment 141706 [details]
One column shortcuts

Created attachment 141707 [details]
One column settings

Created attachment 141709 [details]
Three column settings

Created attachment 141710 [details]
Two (balanced) column settings

[Alexander Pavlov (apavlov)]

(In reply to comment #0)
> Right-docked inspector should show shortcuts/settings in one column.

Shouldn't this depend on the actual width of the inspector WebView? Seems like if it is short but wide, it should also use several columns...

Created attachment 141714 [details]
Patch

Surely, number of columns depends on container width.

[Alexander Pavlov (apavlov)]

Comment on attachment 141714 [details]
Patch

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

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:81
> +        for (var i = 0 ; i < orderedSections.length; ++i) {

no braces around single-line blocks

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:131
> +        var i, keyCell, line;

Why extract all the variable declarations?

[Pavel Feldman]

Why? I don't think it makes sense. My docket-to-the-right inspector has enough real estate to show two columns.

[Andrey Kosyakov]

Comment on attachment 141714 [details]
Patch

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

> Source/WebCore/inspector/front-end/SettingsScreen.js:141
> +    _appendSection: function(name, container)

Do we ever pass different containers to append section? Looks like making container a member will simplify things.

Comment on attachment 141714 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SettingsScreen.js:141
>> +    _appendSection: function(name, container)
> 
> Do we ever pass different containers to append section? Looks like making container a member will simplify things.

Adding members for the sake of simplicity is not a good practice.

Adding temporary members used only in constructor lifetime is a bad practice.

IMHO nested function will match better.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:81
>> +        for (var i = 0 ; i < orderedSections.length; ++i) {
> 
> no braces around single-line blocks

Fixed. Thanks.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:131
>> +        var i, keyCell, line;
> 
> Why extract all the variable declarations?

To avoid accidental duplicate declaration.

Variable declaration at any place inside scope is semantically equivalent to declaration at the beginning of context.
So explicit declaration at the beginning makes code much more clear.

Quote: "...the isolated scope... is created only by execution contexts with 'function' code type... in contrast with C/C++... the block of for loop... does not create a local context".
Reference: http://ecma262-5.com/ELS5_Section_10.htm#Section_10.3

Created attachment 141860 [details]
Patch

@pfeldman this patch doesn't oblige right-docked help screen to be 1-column. If there is enough space, it will be 2+ columned.

Created attachment 141870 [details]
Patch

Created attachment 141882 [details]
More compact settings columns

@pfeldman Shrunk width of column for settings screen. Please take a look at new screenshot.

[Yury Semikhatsky]

Comment on attachment 141714 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:131
>>> +        var i, keyCell, line;
>> 
>> Why extract all the variable declarations?
> 
> To avoid accidental duplicate declaration.
> 
> Variable declaration at any place inside scope is semantically equivalent to declaration at the beginning of context.
> So explicit declaration at the beginning makes code much more clear.
> 
> Quote: "...the isolated scope... is created only by execution contexts with 'function' code type... in contrast with C/C++... the block of for loop... does not create a local context".
> Reference: http://ecma262-5.com/ELS5_Section_10.htm#Section_10.3

Despite the fact that all variables are equally visible in the scope of a function body, inspector coding convention is to declare variables at the places where they are initialized.

[Yury Semikhatsky]

(In reply to comment #11)
> 
> To avoid accidental duplicate declaration.
> 
Closure compiler will catch that.

(In reply to comment #18)
> (In reply to comment #11)
> > 
> > To avoid accidental duplicate declaration.
> > 
> Closure compiler will catch that.

Unfortunately, not. Closure silently eats duplicate var declarations.

Created attachment 141924 [details]
Patch

(In reply to comment #17)
> (From update of attachment 141714 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141714&action=review
> 
> >>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:131
> >>> +        var i, keyCell, line;
> >> 
> >> Why extract all the variable declarations?
> > 
> > To avoid accidental duplicate declaration.
> > 
> > Variable declaration at any place inside scope is semantically equivalent to declaration at the beginning of context.
> > So explicit declaration at the beginning makes code much more clear.
> > 
> > Quote: "...the isolated scope... is created only by execution contexts with 'function' code type... in contrast with C/C++... the block of for loop... does not create a local context".
> > Reference: http://ecma262-5.com/ELS5_Section_10.htm#Section_10.3
> 
> Despite the fact that all variables are equally visible in the scope of a function body, inspector coding convention is to declare variables at the places where they are initialized.

Fixed.

[Andrey Kosyakov]

Comment on attachment 141924 [details]
Patch

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

LGTM with a couple of nits.

> Source/WebCore/inspector/front-end/SettingsScreen.js:51
> +    var appendSection = function(name) {

why not just function appendSection(name)?

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:81
> +            orderedSections[i].renderSection(container.createChild("div", "help-block"));

So we have one help-block per section. Can this be moved into renderSection() then? If so, I would also consider orderedSections.forEach(renderSection, this)

Comment on attachment 141924 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SettingsScreen.js:51
>> +    var appendSection = function(name) {
> 
> why not just function appendSection(name)?

I've done so to make it more uniform with:
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone=Function_Declarations_Within_Blocks#Function_Declarations_Within_Blocks

Fixed.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:81
>> +            orderedSections[i].renderSection(container.createChild("div", "help-block"));
> 
> So we have one help-block per section. Can this be moved into renderSection() then? If so, I would also consider orderedSections.forEach(renderSection, this)

OK.

Created attachment 141965 [details]
Patch

[Andrey Kosyakov]

Committed r117233: <http://trac.webkit.org/changeset/117233>