Bug 149120 - Web Inspector: Preferences for Text Editor behavior
Summary: Web Inspector: Preferences for Text Editor behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 149284
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-14 11:04 PDT by Joseph Pecoraro
Modified: 2017-01-03 14:28 PST (History)
11 users (show)

See Also:


Attachments
Patch (109.30 KB, patch)
2016-09-10 01:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (58.20 KB, image/png)
2016-09-10 01:38 PDT, Devin Rousso
no flags Details
[Image] Sublime Text indentation settings (94.64 KB, image/png)
2016-09-10 12:37 PDT, Nikita Vasilyev
no flags Details
Patch (109.02 KB, patch)
2016-09-10 23:24 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (13.71 KB, patch)
2016-09-13 17:15 PDT, Devin Rousso
drousso: review-
Details | Formatted Diff | Diff
Patch (124.57 KB, patch)
2016-09-14 15:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (50.95 KB, image/png)
2016-09-14 15:38 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (949.34 KB, application/zip)
2016-09-14 16:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.14 MB, application/zip)
2016-09-14 16:30 PDT, Build Bot
no flags Details
Patch (125.33 KB, patch)
2016-09-14 16:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (125.14 KB, patch)
2016-09-15 17:15 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (216.31 KB, image/png)
2016-09-15 17:16 PDT, Devin Rousso
no flags Details
[Image] Simplified on/off settings (183.19 KB, image/png)
2016-09-15 17:26 PDT, Matt Baker
no flags Details
[Image] Settings icon on the right (212.38 KB, image/png)
2016-09-16 00:38 PDT, Devin Rousso
no flags Details
Patch (102.36 KB, patch)
2016-10-20 17:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (101.26 KB, patch)
2016-10-26 17:10 PDT, Devin Rousso
timothy: review+
Details | Formatted Diff | Diff
Patch (98.53 KB, patch)
2016-10-28 17:31 PDT, Devin Rousso
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (98.65 KB, patch)
2016-10-28 17:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-09-14 11:04:10 PDT
* SUMMARY
When adding a location for global preferences, some useful preferences would be to modify Text Editor behavior:

Some examples would be:

  - tab/indent size (default 4 spaces)
  - line wrapping (default off)
  - show whitespace characters (default off)
Comment 1 Radar WebKit Bug Importer 2015-09-14 11:06:24 PDT
<rdar://problem/22686894>
Comment 2 Devin Rousso 2016-09-10 01:37:43 PDT
Created attachment 288484 [details]
Patch
Comment 3 Devin Rousso 2016-09-10 01:38:18 PDT
Created attachment 288485 [details]
[Image] After Patch is applied

Obviously not a perfect representation, but it's a start :)
Comment 4 Nikita Vasilyev 2016-09-10 12:37:40 PDT
Created attachment 288490 [details]
[Image] Sublime Text indentation settings

Sublime Text, among many other code editors, has text indentation settings at the top bottom corner of the editor window.

I'd prefer Web Inspector to have a similar settings menu at the top left corner, right before "{}" and [T] icons.
Comment 5 Nikita Vasilyev 2016-09-10 12:50:07 PDT
(In reply to comment #3)
> Created attachment 288485 [details]
> [Image] After Patch is applied
> 
> Obviously not a perfect representation, but it's a start :)

I don't think any of settings on the screenshot should be in the settings tab.
I think these settings should be accessible from a text editor view.

I don't think we should have a setting to hide line numbers. They don't take too much space. Besides, where do we show breakpoints?

I think we should always match brackets (and parenthesis). I don't think we should have a setting for this.

This doesn't allow to change tab character width. See Sublime Text settings above. Tab character width can be controlled via `tab-size` CSS property. We currently use `tab-size: 4`.
Comment 6 Devin Rousso 2016-09-10 22:58:18 PDT
(In reply to comment #5)
> I don't think we should have a setting to hide line numbers. They don't take
> too much space. Besides, where do we show breakpoints?

Good point.  I did not think of that.

> I think we should always match brackets (and parenthesis). I don't think we
> should have a setting for this.

Yeah, that's fair.  I just figured that if we were adding settings we may as well let the user configure what they wanted.  But this may be a bit much.

> This doesn't allow to change tab character width. See Sublime Text settings
> above. Tab character width can be controlled via `tab-size` CSS property. We
> currently use `tab-size: 4`.

I completely forgot about "tab-size"!  I don't agree that we should display it on the TextEditor view, however, since I don't think we have the space for it.  Considering how many items are already in the navigation bar (Navigation sidebar, back/forward, Path components, {}, [T], [C], Details sidebar), there isn't much room for another editor, especially when both sidebars are shown (I often see the Path components be truncated on my 21" monitor with WebInspector already being half the page).  I don't think that many user's will want to change indentation settings per file (which would introduce it's own set of complications), so I think it makes more sense to keep these settings in SettingsTabContentView.
Comment 7 Devin Rousso 2016-09-10 23:24:33 PDT
Created attachment 288524 [details]
Patch
Comment 8 Nikita Vasilyev 2016-09-12 13:48:30 PDT
(In reply to comment #6)
> I don't agree that we should display
> it on the TextEditor view, however, since I don't think we have the space
> for it.  Considering how many items are already in the navigation bar
> (Navigation sidebar, back/forward, Path components, {}, [T], [C], Details
> sidebar), there isn't much room for another editor, especially when both
> sidebars are shown (I often see the Path components be truncated on my 21"
> monitor with WebInspector already being half the page).  I don't think that
> many user's will want to change indentation settings per file (which would
> introduce it's own set of complications), so I think it makes more sense to
> keep these settings in SettingsTabContentView.

Screenshot from IRC: https://i.imgur.com/4ZkH921.png

As a first iteration, I think it's fine to have these preferences in the Settings tab.
If we find some of them commonly used (e.g. text wrapping), we may pluck them from
the Settings tab and move to the editor's navigation bar.
Comment 9 Devin Rousso 2016-09-13 17:15:55 PDT
Created attachment 288752 [details]
Patch

So I tried reworking _generateCallFramesSection to only resolve once every scope has updated all of its properties (thus only creating DetailsSection instances if a scope has at least one property), but it causes a noticeable delay to the rendering of the scope chain.  This is most likely due to the sheer number of calls to resolve PropertyDescriptor values for RemoteObject.  I think that a current "middle-ground" solution may be to have placeholder elements be added to the DOM (basically blank <div>) that we can remove if there are no properties or replace with a DetailsSection.
Comment 10 Devin Rousso 2016-09-13 17:26:29 PDT
Comment on attachment 288752 [details]
Patch

OK whoops totally uploaded this patch to the wrong bug ⚆_⚆
Comment 11 Joseph Pecoraro 2016-09-13 21:49:25 PDT
Comment on attachment 288524 [details]
Patch

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

r- because of the leak.

I didn't look closely at the refactoring for TabItems, so that will need a closer review.

> Source/WebInspectorUI/ChangeLog:8
> +        * Localizations/en.lproj/localizedStrings.js:

LocalizedStrings should not be treated as binary. Either someone checked in a non-utf8 version, or you updated it with a non-utf8 version.

> Source/WebInspectorUI/ChangeLog:45
> +        * UserInterface/Views/ConsoleTabContentView.js:
> +        (WebInspector.ConsoleTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.
> +
> +        * UserInterface/Views/DebuggerTabContentView.js:
> +        (WebInspector.DebuggerTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.
> +
> +        * UserInterface/Views/ElementsTabContentView.js:
> +        (WebInspector.ElementsTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.

I would combine the 8 repeats of this into one group with the 1 comment to avoid the duplication and wasted newlines. Everyone has their own ChangeLog style. I suggest not being afraid to change the order of lines in here to make an easier to read and still be useful for reviewers.

> Source/WebInspectorUI/UserInterface/Base/Setting.js:107
> +WebInspector.settings = Object.create(null);
> +WebInspector.settings.enableLineWrapping = new WebInspector.Setting("enable-line-wrapping", false);
> +WebInspector.settings.indentUnit = new WebInspector.Setting("indent-unit", 4);

For this kind of thing I think it reads easier without the duplication:

    WebInspector.settings = {
        enableLineWrapping: new WebInspector.Setting(...),
        ...
    };

The Object.create(null) is probably overkill here, since we won't be looking up keys in the object and have to avoid things like "toString".

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:74
> +            let editorContainer = container.createChild("div", "setting-editor");

With all the TextEditor stuff, it took me a while to realize that the use of "editor" in here wasn't a TextEditor. Maybe we should call it a "control", in common with "form control". Or "widget".

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:54
> +        WebInspector.settings.indentWithTabs.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("indentWithTabs", WebInspector.settings.indentWithTabs.value);
> +        });
> +
> +        WebInspector.settings.indentUnit.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("indentUnit", WebInspector.settings.indentUnit.value);
> +        });
> +
> +        WebInspector.settings.enableLineWrapping.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("lineWrapping", WebInspector.settings.enableLineWrapping.value);
> +        });

This would cause every TextEditor that is created to never be garbage collected.

A WebInspector.Object's list of event listeners & targets are lists of strong references. When a View instance adds itself as an event listener on a _global object_. It should remember to remove itself as an event listener from that global object when the View is closed. Otherwise, the View will never be shown again, but will continue to be kept alive because the global object stays alive, and its list of listeners contains the non-visible View.

Here the global WebInspector.Object are each of the `WebInspector.settings.X`. And the listener function captures `this` keeping the `TextEditor` view alive.

For ContentViews we solve this through a very easy pattern. The ContentView interface has shown(), hidden(), closed() methods to be called during the view's lifetime. TextEditor is not a ContentView, but it is always owned by ContentViews. And those ContentViews should appropriately be calling textEditor.close() in closed(). And it looks like they do. However, TextContentView calls _textEditor.close() which doesn't even exist, so that would be a runtime error right now!

Lets do a few things:

  1. Add a TextEditor.prototype.close to detach these listeners
  2. Appropriately call super.close() in SourceCodeTextEditor.prototype.close
  3. Ideally we would update some of the ContentView's closed() methods to call super.closed() even though the base is empty.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:127
> +        let linePrefixText = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            linePrefixText = "\t";

Maybe we should have a global helper for this, since it seems like it is and will be a common operation:

    WebInspector.indentString = function()
    {
        if (WebInspector.settings.indentWithTabs.value)
            return "\t";
        return " ".repeat(WebInspector.settings.indentUnit.value);
    }

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:35
> +        if (!indentString) {
> +            if (WebInspector.settings.indentWithTabs.value)

This is incorrect. EsprimaFormatter lives in a Worker, and does not have access to the WebInspector namespace.

In this code, this should remain a parameter with a reasonable default. In WebInspector land, all of the callers use workerProxy.formatJavaScript(...). They should consult the setting for the appropriate indentString (which I see they do above).

I suppose our tests all pass because they provide an indentString to formatJavaScript. However the WebInspectorUI/Tools/Formatting/index.html tool would probably fail to pretty print anything. Either way, easy fix by just not changing this file.
Comment 12 Devin Rousso 2016-09-14 15:37:43 PDT
Created attachment 288871 [details]
Patch
Comment 13 Devin Rousso 2016-09-14 15:38:17 PDT
Created attachment 288873 [details]
[Image] After Patch is applied
Comment 14 Build Bot 2016-09-14 16:27:06 PDT
Comment on attachment 288871 [details]
Patch

Attachment 288871 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2074939

New failing tests:
inspector/css/generate-css-rule-string.html
Comment 15 Build Bot 2016-09-14 16:27:09 PDT
Created attachment 288887 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 16 Build Bot 2016-09-14 16:30:10 PDT
Comment on attachment 288871 [details]
Patch

Attachment 288871 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2074944

New failing tests:
inspector/css/generate-css-rule-string.html
Comment 17 Build Bot 2016-09-14 16:30:13 PDT
Created attachment 288889 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 18 Devin Rousso 2016-09-14 16:34:18 PDT
Created attachment 288890 [details]
Patch
Comment 19 Joseph Pecoraro 2016-09-14 20:02:30 PDT
Comment on attachment 288890 [details]
Patch

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

r- for the Tab Bar issue.

When I apply this there are very weird issues with when the New Tab button and the Settings Tab button are showing. They both always occupy the same space at the right of the tab bar. So if the button is the settings tab button, to instead get the new tab button you have to click the gear and then the new tab button appears. Likewise if the button is the new tab button but you want the settings tab button. This is definitely confusing. Both should always be visible.

In old demos from Tim Hatcher (April 2015) the setting tab button was pinned to the left, and worked fine with tab modifications. I think that makes a lot of sense.

Also, I think clicking the setting tab button should just show the SettingTabContentView and highlight the Setting tab button as the actively selected tab. It should not a new Setting tab and select that tab. I think this is very important. If the user wants to go to settings that should always be in the same spot and behave as they expect (the always available tab at the left, the settings gear button). If we start making it a "tab" then the user needs to search their list of tabs for the setting tab, and we are introducing a lot of unnecessary friction.

For this reason I avoided reviewing the GeneralTabItem / PinnedTabBarItem refactoring, because I don't think its giving us the behavior we want. We want the Settings tab bar item to behave like a normal tab bar item, but with a few special properties: (1) always be pinned to the left, (2) always have the small style of just the icon, (3) the tab bar item cannot be "closed" the user should just switch to another tab. Note we are always guaranteed to have at least one other tab (the new tab tab).

---

Thanks for adding super across ContentViews for shown/hidden/closed. I wonder if that fixes any subtle bugs!

> Source/WebInspectorUI/UserInterface/Base/Main.js:233
> +    function setTabSize() {
> +        document.body.style.tabSize = WebInspector.settings.indentUnit.value;
> +    }
> +    WebInspector.settings.indentUnit.addEventListener(WebInspector.Setting.Event.Changed, setTabSize);
> +    setTabSize();

It is not clear to me what the intent of what this setting is for.

CodeMirror has a few settings:
https://codemirror.net/doc/manual.html

    - tabSize - the number of spaces a tab represents
    - indentUnit - the number of spaces to auto-indent when editing and you insert a newline and expect indentation
    - indentWithTabs - with the auto-indent when editing, replace the leading groups of `tabSize` spaces with a tab character.

Setting the tabSize on document.body does not affect CodeMirror. It affects anywhere we show code (Debugger Popover for a Function) that is not in a CodeMirror editor.

Likewise with a setting named "Indent width" it is not clear to me that this is actually the tab width. I think we can assume safely that users mostly expect their indentation to be a single tab. So we could get away with a single setting ("Tab width"). But maybe we should have settings for both.

Xcode even breaks this out into 3 options:

    Prefer ident using (Spaces | Tabs)
    Tab width: 4
    Indent width: 4

> Source/WebInspectorUI/UserInterface/Base/Main.js:1053
> +WebInspector.indentString = function() {

Style: Brace on newline.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:361
> +    CodeMirror.defineOption("showWhitespaceCharacters", false, function(cm, val, old) {

Style: We shouldn't abbreviate names like "val". That said, "cm" is fine, its canonical.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:55
> +            console.error("Unkown PinnedTabBarItem mode");

Typo: Unkown

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:817
> +        let indentString = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            indentString = "\t";

Nit: let indentString = WebInspector.indentString();

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:839
> +        let indentString = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            indentString = "\t";

Nit: let indentString = WebInspector.indentString();
Comment 20 Devin Rousso 2016-09-15 00:42:03 PDT
Comment on attachment 288890 [details]
Patch

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

I agree that the action to view settings is very simple, but I don't think we always want to display a gear icon on a pinned settings tab.  I think that users won't change their settings that often, so making it always visible would just clutter things.

Also, do you have a link to the mock from Tim?  I think it may help to just be able to see what he was talking about :)

>> Source/WebInspectorUI/UserInterface/Base/Main.js:233
>> +    setTabSize();
> 
> It is not clear to me what the intent of what this setting is for.
> 
> CodeMirror has a few settings:
> https://codemirror.net/doc/manual.html
> 
>     - tabSize - the number of spaces a tab represents
>     - indentUnit - the number of spaces to auto-indent when editing and you insert a newline and expect indentation
>     - indentWithTabs - with the auto-indent when editing, replace the leading groups of `tabSize` spaces with a tab character.
> 
> Setting the tabSize on document.body does not affect CodeMirror. It affects anywhere we show code (Debugger Popover for a Function) that is not in a CodeMirror editor.
> 
> Likewise with a setting named "Indent width" it is not clear to me that this is actually the tab width. I think we can assume safely that users mostly expect their indentation to be a single tab. So we could get away with a single setting ("Tab width"). But maybe we should have settings for both.
> 
> Xcode even breaks this out into 3 options:
> 
>     Prefer ident using (Spaces | Tabs)
>     Tab width: 4
>     Indent width: 4

Clearly I misunderstood this, because I thought that CodeMirror used actual tab characters.  It seems like it inserts the corresponding number of spaces as the "tabSize" option value.  I think your suggestion makes sense :)

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:361
>> +    CodeMirror.defineOption("showWhitespaceCharacters", false, function(cm, val, old) {
> 
> Style: We shouldn't abbreviate names like "val". That said, "cm" is fine, its canonical.

I was following the signature defined in codemirror.js, but I agree.  I'll change it.
Comment 21 Devin Rousso 2016-09-15 17:15:59 PDT
Created attachment 289019 [details]
Patch
Comment 22 Devin Rousso 2016-09-15 17:16:18 PDT
Created attachment 289020 [details]
[Image] After Patch is applied
Comment 23 Matt Baker 2016-09-15 17:26:04 PDT
(In reply to comment #22)
> Created attachment 289020 [details]
> [Image] After Patch is applied

Looking very nice! A couple things:

- The gear should be on the right, so that it isn't given so much priority in the UI.
- Simple on/off settings don't need ": Visible" text (see screenshot)
Comment 24 Matt Baker 2016-09-15 17:26:54 PDT
Created attachment 289022 [details]
[Image] Simplified on/off settings

Also added 6px margin between checkbox and label.
Comment 25 Matt Baker 2016-09-15 17:27:56 PDT
(In reply to comment #24)
> Created attachment 289022 [details]
> [Image] Simplified on/off settings
> 
> Also added 6px margin between checkbox and label.

I'm not convinced we should use a two column layout, where labels are right aligned and values left aligned.
Comment 26 Devin Rousso 2016-09-15 17:29:34 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > Created attachment 289022 [details]
> > [Image] Simplified on/off settings
> > 
> > Also added 6px margin between checkbox and label.
> 
> I'm not convinced we should use a two column layout, where labels are right
> aligned and values left aligned.

I am trying to follow the Syntax/Indentation settings in Xcode.  The issue is that Xcode has so many more settings (that are usually grouped by the text on the left) that we don't have (either we can't do it because of CodeMirror or because it isn't very useful).
Comment 27 Devin Rousso 2016-09-16 00:38:22 PDT
Created attachment 289044 [details]
[Image] Settings icon on the right

So I tried just moving the element in the DOM tree to the left (I'm lazy and tired...sue me), and I (surprisingly) don't like how this looks.  I think that having two pinned tab icons next to each-other is very confusing.

Also, the logistics of getting another pinned tab that actually represents a ContentView and not just a button onto the tab bar (and to the right of the new tab item) looks to be harder than I thought.

I think it makes the most sense to either leave the settings where it currently is (on the left) or switch the positions of the two pinned tab items (new tab is left and settings is right).
Comment 28 Devin Rousso 2016-10-20 17:33:07 PDT
Created attachment 292292 [details]
Patch
Comment 29 Devin Rousso 2016-10-26 17:10:48 PDT
Created attachment 292976 [details]
Patch
Comment 30 Timothy Hatcher 2016-10-28 16:23:36 PDT
Comment on attachment 292976 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:433
> +    if (openTabTypes.includes("settings")) {
> +        openTabTypes.remove("settings");
> +        this._openTabsSetting.value = openTabTypes;
> +    }

Is this still needed?

> Source/WebInspectorUI/UserInterface/Base/Main.js:1041
> +

Can remove this line sine the return comes after a conditional return.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:60
> +    static modeInfo(mode)
> +    {
> +        switch (mode) {
> +        case WebInspector.PinnedTabBarItem.Mode.NewTab:
> +            return {
> +                image: "Images/NewTabPlus.svg",
> +                title: WebInspector.UIString("Create a new tab"),
> +            };
> +
> +        case WebInspector.PinnedTabBarItem.Mode.Settings:
> +            return {
> +                image: "Images/Gear.svg",
> +                title: WebInspector.UIString("Open Settings"),
> +            };
> +
> +        default:
> +            console.error("Unknown PinnedTabBarItem mode");
> +            return {};
> +        }
> +    }

This is kind of a layering violation. These details could be passed into the constructor from the separate views or whatever.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:73
> +        if (this._mode === WebInspector.PinnedTabBarItem.Mode.NewTab) {
> +            const shouldAnimate = true;
> +            WebInspector.showNewTabTab(shouldAnimate);
> +        }

The only real use of mode seems to be here. This could be handled externally with an event listener.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:198
> +        let wasLastNormalTab = this._tabBarItems.indexOf(tabBarItem) === this._tabBarItems.length - 3;

This magic number should have a comment or ideally be computed. I know the old code was also a little magical too.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:348
> +            tabBarItem = this._tabBarItems[this._tabBarItems.length - 3];

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:648
> +        newIndex = Math.min(this._tabBarItems.length - 3, newIndex);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:156
> +        console.assert(this._recentTabContentViews.length === this._tabBar.tabBarItems.length - 2);

It would be good to have a central place to check this and not have these numbers sprinkled around. Why do we not need the check for this._tabBar.newTabItem anymore?
Comment 31 Devin Rousso 2016-10-28 17:28:13 PDT
Comment on attachment 292976 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:156
>> +        console.assert(this._recentTabContentViews.length === this._tabBar.tabBarItems.length - 2);
> 
> It would be good to have a central place to check this and not have these numbers sprinkled around. Why do we not need the check for this._tabBar.newTabItem anymore?

The checks for newTabItem were originally used to check whether there were any pinned tabs in the TabBar.  Since the newTabItem was given to the TabBar by its "parent" TabBrowser, it wasn't guaranteed that it existed on tabBar.  Now, since it is always created by the TabBar, we just need to check whether the number of recent (meaning opened) tabs is equal to the number of normal tab bar items inside the TabBar.  Since there are two pinned tab bar items, I subtract 2 from that count.
Comment 32 Devin Rousso 2016-10-28 17:31:08 PDT
Created attachment 293255 [details]
Patch
Comment 33 WebKit Commit Bot 2016-10-28 17:32:23 PDT
Comment on attachment 293255 [details]
Patch

Rejecting attachment 293255 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 293255, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
InspectorUI/UserInterface/Views/TabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TextEditor.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2396304
Comment 34 Devin Rousso 2016-10-28 17:45:34 PDT
Created attachment 293259 [details]
Patch
Comment 35 WebKit Commit Bot 2016-10-28 18:20:37 PDT
Comment on attachment 293259 [details]
Patch

Clearing flags on attachment: 293259

Committed r208091: <http://trac.webkit.org/changeset/208091>
Comment 36 WebKit Commit Bot 2016-10-28 18:20:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Joseph Pecoraro 2017-01-03 14:28:40 PST
Comment on attachment 293259 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorOverrides.css:178
> +.show-invalid-characters .CodeMirror .cm-invalidchar {
>      display: none;
>  }

This appears to be the inverse of what we want. I'll fix this up.