Bug 149173 - Web Inspector: command-click in NewTabContentView should open new tab without switching to it
Summary: Web Inspector: command-click in NewTabContentView should open new tab without...
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: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-15 11:27 PDT by BJ Burg
Modified: 2015-09-15 16:44 PDT (History)
7 users (show)

See Also:


Attachments
Proposed fix (5.64 KB, patch)
2015-09-15 13:46 PDT, BJ Burg
joepeck: review+
bburg: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-09-15 11:27:40 PDT
This is how the home screen grids work in FF and Safari.
Comment 1 Radar WebKit Bug Importer 2015-09-15 11:28:04 PDT
<rdar://problem/22705409>
Comment 2 BJ Burg 2015-09-15 13:46:41 PDT
Created attachment 261227 [details]
Proposed fix
Comment 3 Joseph Pecoraro 2015-09-15 14:31:58 PDT
Comment on attachment 261227 [details]
Proposed fix

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:510
> +WebInspector.createNewTabWithType = function(tabType, options={})

In the one other place I think we have this right now, we have spaces. I still favor no spaces. Lets decide on IRC.

> Source/WebInspectorUI/UserInterface/Base/Main.js:520
> +    let insertionIndex = referencedView ? this.tabBar.tabBarItems.indexOf(referencedView.tabBarItem) : null;

Nit: I think `undefined` may be better than `null`. Null makes me think an object should go here, but it should be a number (or a default value), which null is not typically associated with.

> Source/WebInspectorUI/UserInterface/Views/NewTabContentView.js:114
> +        let tabItemElements = Array.from(this.element.querySelectorAll("." + WebInspector.NewTabContentView.TabItemStyleClassName));

Nit: This could be a private function, since it is done in multiple places and is easy to get wrong.
Comment 4 BJ Burg 2015-09-15 16:42:47 PDT
Comment on attachment 261227 [details]
Proposed fix

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:520
>> +    let insertionIndex = referencedView ? this.tabBar.tabBarItems.indexOf(referencedView.tabBarItem) : null;
> 
> Nit: I think `undefined` may be better than `null`. Null makes me think an object should go here, but it should be a number (or a default value), which null is not typically associated with.

OK
Comment 5 BJ Burg 2015-09-15 16:44:43 PDT
Committed r189828: <http://trac.webkit.org/changeset/189828>