Bug 86367 - Web Inspector: Use CSS columns feature for HelpScreen contents.
Summary: Web Inspector: Use CSS columns feature for HelpScreen contents.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: eustas.bug
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-14 06:11 PDT by eustas.bug
Modified: 2012-10-12 04:05 PDT (History)
12 users (show)

See Also:


Attachments
One column shortcuts (155.37 KB, image/png)
2012-05-14 06:11 PDT, eustas.bug
no flags Details
One column settings (135.96 KB, image/png)
2012-05-14 06:12 PDT, eustas.bug
no flags Details
Three column settings (140.02 KB, image/png)
2012-05-14 06:12 PDT, eustas.bug
no flags Details
Two (balanced) column settings (156.13 KB, image/png)
2012-05-14 06:13 PDT, eustas.bug
no flags Details
Patch (14.51 KB, patch)
2012-05-14 06:23 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (14.55 KB, patch)
2012-05-14 23:22 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (14.78 KB, patch)
2012-05-15 00:08 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
More compact settings columns (134.55 KB, image/png)
2012-05-15 01:05 PDT, eustas.bug
no flags Details
Patch (15.69 KB, patch)
2012-05-15 05:04 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2012-05-15 07:24 PDT, eustas.bug
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-05-14 06:11:21 PDT
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.
Comment 1 eustas.bug 2012-05-14 06:11:59 PDT
Created attachment 141706 [details]
One column shortcuts
Comment 2 eustas.bug 2012-05-14 06:12:24 PDT
Created attachment 141707 [details]
One column settings
Comment 3 eustas.bug 2012-05-14 06:12:44 PDT
Created attachment 141709 [details]
Three column settings
Comment 4 eustas.bug 2012-05-14 06:13:17 PDT
Created attachment 141710 [details]
Two (balanced) column settings
Comment 5 Alexander Pavlov (apavlov) 2012-05-14 06:17:40 PDT
(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...
Comment 6 eustas.bug 2012-05-14 06:23:31 PDT
Created attachment 141714 [details]
Patch
Comment 7 eustas.bug 2012-05-14 06:26:05 PDT
Surely, number of columns depends on container width.
Comment 8 Alexander Pavlov (apavlov) 2012-05-14 06:30:30 PDT
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?
Comment 9 Pavel Feldman 2012-05-14 08:36:54 PDT
Why? I don't think it makes sense. My docket-to-the-right inspector has enough real estate to show two columns.
Comment 10 Andrey Kosyakov 2012-05-14 10:22:05 PDT
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 11 eustas.bug 2012-05-14 22:48:07 PDT
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
Comment 12 eustas.bug 2012-05-14 23:22:51 PDT
Created attachment 141860 [details]
Patch
Comment 13 eustas.bug 2012-05-14 23:32:27 PDT
@pfeldman this patch doesn't oblige right-docked help screen to be 1-column. If there is enough space, it will be 2+ columned.
Comment 14 eustas.bug 2012-05-15 00:08:44 PDT
Created attachment 141870 [details]
Patch
Comment 15 eustas.bug 2012-05-15 01:05:40 PDT
Created attachment 141882 [details]
More compact settings columns
Comment 16 eustas.bug 2012-05-15 01:06:18 PDT
@pfeldman Shrunk width of column for settings screen. Please take a look at new screenshot.
Comment 17 Yury Semikhatsky 2012-05-15 02:30:04 PDT
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.
Comment 18 Yury Semikhatsky 2012-05-15 02:31:14 PDT
(In reply to comment #11)
> 
> To avoid accidental duplicate declaration.
> 
Closure compiler will catch that.
Comment 19 eustas.bug 2012-05-15 03:32:58 PDT
(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.
Comment 20 eustas.bug 2012-05-15 05:04:41 PDT
Created attachment 141924 [details]
Patch
Comment 21 eustas.bug 2012-05-15 05:05:53 PDT
(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.
Comment 22 Andrey Kosyakov 2012-05-15 05:23:25 PDT
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 23 eustas.bug 2012-05-15 07:19:50 PDT
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.
Comment 24 eustas.bug 2012-05-15 07:24:41 PDT
Created attachment 141965 [details]
Patch
Comment 25 Andrey Kosyakov 2012-05-16 01:38:05 PDT
Committed r117233: <http://trac.webkit.org/changeset/117233>