Mac: Make devtools window dockable.

Mac: Make devtools window dockable.

xib change: Deleted NSBox, added NSSplitView instead (with a thin divider and without any child views).

I added the devtools tabcontents to TabContentsController; windows and linux instead add it to the browser window and switch it on every tab change. What I've done makes more sense to me and might work better with a) dragging a tab with docked devtools into a new window and b) toggling fullscreen.

BUG=17368
TEST=
* Inspect element, click the "attach" item in the lower left corner. Devtools should attach to the tab. Click it again, should detach. Re-attach, switch tabs. Should only be in the tab it was attached to. Open another devtools window in another tab, should be attached there. Drag tab with attached devtools into a new window, should work.

* Inspect element with docked devtools, close devtools, inspect another element. devtools should open with the same size it had when it was closed, and should still be docked.

* Hover link with docked devtools. Status bubble should not overlap devtools.

* Go to http://www.pagetutor.com/keeper/http_authentication/index.html and make sure the http auth sheet still shows up

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35576

[pink (ping after 24hrs), 2010-01-04 22:23:15]

Per IRC, this feels like the wrong place to put the concept of the dev tools
panel. The TabContentsController is self-contained for displaying the contents
of a TabContents, and now it has to handle something unrelated to that task (ui
for dev tools). It seems like it's too low, and maybe a new class should wrap
the NSSPlitView and the TabContentsController to impl the functionality of the
attached dev tools.

Do others agree, or am I on crack?

http://codereview.chromium.org/526001/diff/3012/2029
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/3012/2029#newcode19
chrome/browser/cocoa/tab_contents_controller.h:19: // the render widget host
view.
this comment should be updated for the dev tools attaching.

http://codereview.chromium.org/526001/diff/3012/2029#newcode52
chrome/browser/cocoa/tab_contents_controller.h:52: // Shows |devtolsContents| in
a split view, or removes the bottom view in the
devtoolsContents

http://codereview.chromium.org/526001/diff/3012/2030
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/3012/2030#newcode81
chrome/browser/cocoa/tab_contents_controller.mm:81: else /* count == 2 */
dcheck(count==2)?

http://codereview.chromium.org/526001/diff/3012/2030#newcode82
chrome/browser/cocoa/tab_contents_controller.mm:82: [contentsBox_
replaceSubview:[subviews objectAtIndex:1]
multi-line statement means that the else should have braces (that's how mento
likes it), which means the if needs them too.

http://codereview.chromium.org/526001/diff/3012/2030#newcode99
chrome/browser/cocoa/tab_contents_controller.mm:99: webFrame.size.height =
NSHeight([contentsBox_ frame])
operator (-) should go on this line, not next line.

[Nico, 2010-01-04 22:28:34]

Controlling a tab is next to no work (case in point: TabContents.xib didn't
contain anything useful prior to this change :-) ). How about renaming the class
instead? Then again, I can't think of a better name for TabContents + devtools
(TabContentsContainer?), so I left it as is.

Addressed all but one comment. Did not yet address the comment that depends on
how / where this devtools stuff is handled.

http://codereview.chromium.org/526001/diff/3012/2029
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/3012/2029#newcode52
chrome/browser/cocoa/tab_contents_controller.h:52: // Shows |devtolsContents| in
a split view, or removes the bottom view in the
On 2010/01/04 22:23:15, pink wrote:
> devtoolsContents

Done.

http://codereview.chromium.org/526001/diff/3012/2030
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/3012/2030#newcode81
chrome/browser/cocoa/tab_contents_controller.mm:81: else /* count == 2 */
On 2010/01/04 22:23:15, pink wrote:
> dcheck(count==2)?

Done.

http://codereview.chromium.org/526001/diff/3012/2030#newcode82
chrome/browser/cocoa/tab_contents_controller.mm:82: [contentsBox_
replaceSubview:[subviews objectAtIndex:1]
On 2010/01/04 22:23:15, pink wrote:
> multi-line statement means that the else should have braces (that's how mento
> likes it), which means the if needs them too.

Done.

http://codereview.chromium.org/526001/diff/3012/2030#newcode99
chrome/browser/cocoa/tab_contents_controller.mm:99: webFrame.size.height =
NSHeight([contentsBox_ frame])
On 2010/01/04 22:23:15, pink wrote:
> operator (-) should go on this line, not next line.

Bleh, too much java lately. Done.

[viettrungluu, 2010-01-04 22:46:42]

As Nico points out, a |TabContents| doesn't really need to be controlled (being
rather controller-like already).

What a |TabContentsController| really controls is not a |TabContents|, but the
contents of a tab, i.e., the stuff which is switched in and out when switching
tabs. So +1 on renaming |TabContentsController|, but good luck of finding a good
name. (I would argue that it's |TabContents| which is misnamed, but I think it's
safe to say that it's not going to be renamed.)

http://codereview.chromium.org/526001/diff/3012/2029
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/3012/2029#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: -
(void)setDevToolsContents:(TabContents*)devtoolsContents;
I'm wary of:
- things which look like mutators but aren't really (or kind of are);
- things which look like mutators but have lots of side effects (well, maybe I'm
okay with this);
- things which look like mutators but don't have corresponding accessors.
(I'm sure I'm a hypocrite, but anyway....)

http://codereview.chromium.org/526001/diff/3012/2030
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/3012/2030#newcode81
chrome/browser/cocoa/tab_contents_controller.mm:81: else /* count == 2 */
On 2010/01/04 22:28:35, Nico wrote:
> On 2010/01/04 22:23:15, pink wrote:
> > dcheck(count==2)?
> 
> Done.

I assume that that really meant |DCHECK_EQ(2, count);|.

[Nico, 2010-01-04 23:06:40]

http://codereview.chromium.org/526001/diff/3012/2029
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/3012/2029#newcode19
chrome/browser/cocoa/tab_contents_controller.h:19: // the render widget host
view.
On 2010/01/04 22:23:15, pink wrote:
> this comment should be updated for the dev tools attaching.

Done.

http://codereview.chromium.org/526001/diff/3012/2029#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: -
(void)setDevToolsContents:(TabContents*)devtoolsContents;
On 2010/01/04 22:46:42, viettrungluu wrote:
> I'm wary of:
> - things which look like mutators but aren't really (or kind of are);
> - things which look like mutators but have lots of side effects (well, maybe
I'm
> okay with this);
> - things which look like mutators but don't have corresponding accessors.
> (I'm sure I'm a hypocrite, but anyway....)

Renamed to showDevT…

http://codereview.chromium.org/526001/diff/3012/2030
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/3012/2030#newcode81
chrome/browser/cocoa/tab_contents_controller.mm:81: else /* count == 2 */
On 2010/01/04 22:46:42, viettrungluu wrote:
> On 2010/01/04 22:28:35, Nico wrote:
> > On 2010/01/04 22:23:15, pink wrote:
> > > dcheck(count==2)?
> > 
> > Done.
> 
> I assume that that really meant |DCHECK_EQ(2, count);|.

That's what I heard, too :-)

[pfeldman, 2010-01-05 12:13:44]

http://codereview.chromium.org/526001/diff/1014/1023
File chrome/browser/debugger/devtools_manager.cc (left):

http://codereview.chromium.org/526001/diff/1014/1023#oldcode345
chrome/browser/debugger/devtools_manager.cc:345: #if defined OS_MACOSX
Yeah, sorry for not pointing you to this one.

http://codereview.chromium.org/526001/diff/1014/1024
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/526001/diff/1014/1024#newcode36
chrome/browser/debugger/devtools_window.cc:36: return NULL;  // Happens only in
tests.
Removing this is likely to break tests as mentioned in the comment.

[Nico, 2010-01-05 15:07:25]

http://codereview.chromium.org/526001/diff/1014/1024
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/526001/diff/1014/1024#newcode36
chrome/browser/debugger/devtools_window.cc:36: return NULL;  // Happens only in
tests.
On 2010/01/05 12:13:44, pfeldman wrote:
> Removing this is likely to break tests as mentioned in the comment.

Not sure what you're saying here. Do you mean it's good I added that, or is
there a problem with this?

[rohitrao (ping after 24h), 2010-01-05 18:40:07]

Bunch of comments, mostly just places to double-check and make sure things
aren't broken.

I like using the NSSplitView, although I'm a little against putting this
functionality so deep into TabContentsController.  Looking at the infobar for
precedent, we already have TabContents-specific stuff in a completely separate
container.  Why should devtools be any different?

http://codereview.chromium.org/526001/diff/1014/1019
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/1014/1019#newcode27
chrome/browser/cocoa/tab_contents_controller.h:27: IBOutlet NSSplitView*
contentsBox_;
How hard would it be to rename this to splitView_?

http://codereview.chromium.org/526001/diff/1014/1019#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: // split view if
|devtolsContents| is NULL.
Typo: devtolsContents

http://codereview.chromium.org/526001/diff/1014/1020
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1020#newcode16
chrome/browser/cocoa/tab_contents_controller.mm:16: static const int
kDefaultContentsSplitOffset = 400;
This is larger than the minimum height of the window (250px).  Does something
reasonable happen when the window is <400px?

http://codereview.chromium.org/526001/diff/1014/1022
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1022#newcode941
chrome/browser/cocoa/tab_strip_controller.mm:941: [self
updateDevToolsForContents:newContents];
On a tab switch, we adjust views in 3 or 4 different Observe() methods.  Do you
notice any rendering flicker when changing tabs with devtools, similar to what
we get with the infobar?

http://codereview.chromium.org/526001/diff/1014/1022#newcode1617
chrome/browser/cocoa/tab_strip_controller.mm:1617: return [tabContentsArray_
objectAtIndex:tabStripModel_->selected_index()];
This still works even if a tab is currently animating closed, right?  I don't
remember if tabs disappear from the model on animate start or animate end.

http://codereview.chromium.org/526001/diff/1014/1022#newcode1715
chrome/browser/cocoa/tab_strip_controller.mm:1715: int index =
tabStripModel_->GetIndexOfTabContents(contents);
GetIndexOfTabContents() can be called with contents == NULL, right?  I think you
call this explicitly somewhere above, so the answer is probably yes ;)

http://codereview.chromium.org/526001/diff/1014/1025
File ipc/ipc_message.cc (right):

http://codereview.chromium.org/526001/diff/1014/1025#newcode126
ipc/ipc_message.cc:126: }  // namespace IPC
What is this change?  Whitespace? Newline at the end of the file?

[pfeldman, 2010-01-05 18:49:37]

http://codereview.chromium.org/526001/diff/1014/1024
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/526001/diff/1014/1024#newcode36
chrome/browser/debugger/devtools_window.cc:36: return NULL;  // Happens only in
tests.
It is just that my holiday season is not over yet. I was looking at plain diff
at first and got impression that this code is being removed, not added. It looks
right, sorry for noise.

[Nico, 2010-01-05 19:39:49]

http://codereview.chromium.org/526001/diff/1014/1019
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/1014/1019#newcode27
chrome/browser/cocoa/tab_contents_controller.h:27: IBOutlet NSSplitView*
contentsBox_;
On 2010/01/05 18:40:07, rohitrao wrote:
> How hard would it be to rename this to splitView_?

It's certainly doable (i changed this file, the mm file, and the nib), but this
just shows that it's not a good idea to name variables after their type. Renamed
to contentsContainer_.

http://codereview.chromium.org/526001/diff/1014/1019#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: // split view if
|devtolsContents| is NULL.
On 2010/01/05 18:40:07, rohitrao wrote:
> Typo: devtolsContents

Done.

http://codereview.chromium.org/526001/diff/1014/1020
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1020#newcode16
chrome/browser/cocoa/tab_contents_controller.mm:16: static const int
kDefaultContentsSplitOffset = 400;
On 2010/01/05 18:40:07, rohitrao wrote:
> This is larger than the minimum height of the window (250px).  Does something
> reasonable happen when the window is <400px?

No, everything blew up. Excellent catch. Done.

http://codereview.chromium.org/526001/diff/1014/1022
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1022#newcode941
chrome/browser/cocoa/tab_strip_controller.mm:941: [self
updateDevToolsForContents:newContents];
On 2010/01/05 18:40:07, rohitrao wrote:
> On a tab switch, we adjust views in 3 or 4 different Observe() methods.  Do
you
> notice any rendering flicker when changing tabs with devtools, similar to what
> we get with the infobar?

No, I don't see that flicker with docked devtools. Maybe because it's contained
in the FastResize box that also contains the normal tab contents?

(I _do_ see the flicker on the NTP if the bookmark bar is in detached mode, so
when opening the devtools on the NTP, I can still set that flicker. But not if I
have them open on a normal web page with no detached bookmark bar visible)

http://codereview.chromium.org/526001/diff/1014/1022#newcode1617
chrome/browser/cocoa/tab_strip_controller.mm:1617: return [tabContentsArray_
objectAtIndex:tabStripModel_->selected_index()];
On 2010/01/05 18:40:07, rohitrao wrote:
> This still works even if a tab is currently animating closed, right?  I don't
> remember if tabs disappear from the model on animate start or animate end.

Don't know. Added a check for the upper bound, too.

http://codereview.chromium.org/526001/diff/1014/1022#newcode1715
chrome/browser/cocoa/tab_strip_controller.mm:1715: int index =
tabStripModel_->GetIndexOfTabContents(contents);
On 2010/01/05 18:40:07, rohitrao wrote:
> GetIndexOfTabContents() can be called with contents == NULL, right?  I think
you
> call this explicitly somewhere above, so the answer is probably yes ;)

Yes, the function just walks a list and compares elements with the passed-in
contents. Should be safe.

[Nico, 2010-01-05 21:29:11]

I filed http://code.google.com/p/chromium/issues/detail?id=31633 . Hooray?

[pink (ping after 24hrs), 2010-01-05 21:58:28]

LGTM

http://codereview.chromium.org/526001/diff/7002/6006
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/7002/6006#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: // split view if
|devtoolsContents| is NULL.
Can you add the TODO with the bug number here to clean this up, so we see it in
the code as well?

http://codereview.chromium.org/526001/diff/7002/6006#newcode59
chrome/browser/cocoa/tab_contents_controller.h:59: - (CGFloat)devtoolsHeight;
is it |devtools| or |devTools|? the method above is showDevTools... but the
params are always devtools. Be consistent.

[rohitrao (ping after 24h), 2010-01-05 22:05:16]

http://codereview.chromium.org/526001/diff/1014/1022
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1022#newcode1617
chrome/browser/cocoa/tab_strip_controller.mm:1617: return [tabContentsArray_
objectAtIndex:tabStripModel_->selected_index()];
On 2010/01/05 19:39:49, Nico wrote:
> On 2010/01/05 18:40:07, rohitrao wrote:
> > This still works even if a tab is currently animating closed, right?  I
don't
> > remember if tabs disappear from the model on animate start or animate end.
> 
> Don't know. Added a check for the upper bound, too.

Per IRC discussion, we need to use indexFromModelIndex here.

[Nico, 2010-01-05 22:22:39]

Thanks!

http://codereview.chromium.org/526001/diff/1014/1022
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/526001/diff/1014/1022#newcode1617
chrome/browser/cocoa/tab_strip_controller.mm:1617: return [tabContentsArray_
objectAtIndex:tabStripModel_->selected_index()];
On 2010/01/05 22:05:16, rohitrao wrote:
> On 2010/01/05 19:39:49, Nico wrote:
> > On 2010/01/05 18:40:07, rohitrao wrote:
> > > This still works even if a tab is currently animating closed, right?  I
> don't
> > > remember if tabs disappear from the model on animate start or animate end.
> > 
> > Don't know. Added a check for the upper bound, too.
> 
> Per IRC discussion, we need to use indexFromModelIndex here.

Good catch. I thought indexFromModelIndex only refers to the tabArray_, not
tabContentsArray_, but you're right. This could be documented better. Improved
the comments in the .h file.

http://codereview.chromium.org/526001/diff/7002/6006
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/526001/diff/7002/6006#newcode54
chrome/browser/cocoa/tab_contents_controller.h:54: // split view if
|devtoolsContents| is NULL.
On 2010/01/05 21:58:28, pink wrote:
> Can you add the TODO with the bug number here to clean this up, so we see it
in
> the code as well?

Done.

http://codereview.chromium.org/526001/diff/7002/6006#newcode59
chrome/browser/cocoa/tab_contents_controller.h:59: - (CGFloat)devtoolsHeight;
On 2010/01/05 21:58:28, pink wrote:
> is it |devtools| or |devTools|? the method above is showDevTools... but the
> params are always devtools. Be consistent.

There's precedent: The core devtools files are called devtools_foo.cc but
contain classes named DevToolsFoo :-)

Done.

[pink (ping after 24hrs), 2010-01-05 22:34:30]

http://codereview.chromium.org/526001/diff/29/1035
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/526001/diff/29/1035#newcode61
chrome/browser/cocoa/tab_strip_controller.h:61: // |TabStripModel| and this
array are identical.
tabStripModel_? Maybe add that, for instance, they won't be identical when tabs
are animating closed.

[rohitrao (ping after 24h), 2010-01-05 22:38:56]

LGTM

Looking at the other calls to indexFromModelIndex, I think the new DCHECK is ok.

[Nico, 2010-01-05 22:40:18]

Thanks again.

Submitting once the most recent version is through the trybots.

http://codereview.chromium.org/526001/diff/29/1035
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/526001/diff/29/1035#newcode61
chrome/browser/cocoa/tab_strip_controller.h:61: // |TabStripModel| and this
array are identical.
On 2010/01/05 22:34:30, pink wrote:
> tabStripModel_? Maybe add that, for instance, they won't be identical when
tabs
> are animating closed.

Done.