Bug 79518 – Web Inspector: Close TabbedPanes on middle click of tab handle

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

[Dan Beam]

Corresponding webkit bug with patch to implement http://crbug.com/115551

If the feature ends up not being wanted, ignore this.

[Dan Beam]

Created attachment 128780 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 128780 [details]
Patch

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

> Source/WebCore/inspector/front-end/TabbedPane.js:619
> +            if (event.button == 1 || event.target.classList.contains('tabbed-pane-header-tab-close-button'))

Quotes are used instead of apostrophes for string literals across Web Inspector.
Also, to determine if a CSS class is present on an element, you should use event.target.hasStyleClass("tabbed-pane-header-tab-close-button"); for better readability (Element.prototype.hasStyleClass is defined in utilities.js).

[Dan Beam]

Created attachment 128857 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 128857 [details]
Patch

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

You need a ChangeLog entry for WebKit patches as well. It can be put together by the Tools/Scripts/prepare-ChangeLog script. I believe having one, together with the comparison fix (see below), will qualify as a good patch.

> Source/WebCore/inspector/front-end/TabbedPane.js:619
> +            if (event.button == 1 || event.target.hasStyleClass("tabbed-pane-header-tab-close-button"))

Sorry for overlooking this, we use the === comparison whenever possible.

Also, don't forget to set the r? flag on the patches you upload for the review.

[Dan Beam]

Created attachment 128881 [details]
Patch

[Dan Beam]

Created attachment 128882 [details]
Patch

[WebKit Review Bot]

Attachment 128882 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dan Beam]

Created attachment 128895 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 128895 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

ChangeLogs should never have the OOPS! string (there is a presubmit check that will prevent your patch from landing.) pfeldman and vsevik will know better if this functionality is/should be covered with a test. Otherwise the patch looks fine.

[Pavel Feldman]

Comment on attachment 128895 [details]
Patch

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

Thanks for the fix, I'll fix the ChangeLog and land it.

> Source/WebCore/ChangeLog:6
> +        Reviewed by Alexander Pavlov.

To avoid further confusion, let me clarify. ChangeLog must contain "Reviewed by NOBODY (OOPS!)." string. No other OOPS should be there. Reviewer is being filled in upon landing automatically.

> Source/WebCore/inspector/front-end/TabbedPane.js:618
> +        if (this._closeable && (event.button === 1 || event.target.hasStyleClass("tabbed-pane-header-tab-close-button")))

nit: I would make closeButtonSpan tab's field and compare it here.

[Pavel Feldman]

Committed r108966: <http://trac.webkit.org/changeset/108966>