Issue 151150: Add split with devtools container into the browser view.

Issue 151150: Add split with devtools container into the browser view. (Closed)

Created:
11 years, 5 months ago by pfeldman

Modified:
9 years, 7 months ago

Reviewers:
Ben Goodger (Google), yurys, sky

CC:
chromium-reviews_googlegroups.com, darin (slow to review), Ben Goodger (Google)

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Add split with devtools container into the browser view. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19807

Patch Set 2 : '' #

Created: 11 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+61 lines, -10 lines)			Patch
M	chrome/browser/views/frame/browser_view.h	View	1	3 chunks	+10 lines, -0 lines	0 comments	Download
M	chrome/browser/views/frame/browser_view.cc	View	1	15 chunks	+44 lines, -6 lines	0 comments	Download
M	webkit/glue/devtools/js/devtools.css	View	1	1 chunk	+7 lines, -4 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Ben Goodger (Google)

Adding sky to look at SplitView usage. http://codereview.chromium.org/151150/diff/1/2 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/151150/diff/1/2#newcode1542 Line 1542: contents_split_->Layout(); ...

11 years, 5 months ago (2009-07-01 18:20:07 UTC) #2

sky

http://codereview.chromium.org/151150/diff/1/2 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/151150/diff/1/2#newcode1542 Line 1542: contents_split_->Layout(); On 2009/07/01 18:20:08, Ben Goodger wrote: > ...

11 years, 5 months ago (2009-07-01 19:43:38 UTC) #3

pfeldman

Thanks for review! Yury, could you take a look at the rest of the CL? ...

11 years, 5 months ago (2009-07-02 08:37:56 UTC) #4

yurys

On 2009/07/02 08:37:56, pfeldman wrote: > Thanks for review! > > Yury, could you take ...

11 years, 5 months ago (2009-07-02 10:00:55 UTC) #5

On 2009/07/02 08:37:56, pfeldman wrote:
> Thanks for review!
> 
> Yury, could you take a look at the rest of the CL? If it looks good to you,
I'd
> like to commit it. I think I fixed everything on the UI side.
> 
> http://codereview.chromium.org/151150/diff/1/2
> File chrome/browser/views/frame/browser_view.cc (right):
> 
> http://codereview.chromium.org/151150/diff/1/2#newcode1542
> Line 1542: contents_split_->Layout();
> On 2009/07/01 18:20:08, Ben Goodger wrote:
> > Shouldn't the split view be laid out automatically as its bounds change?
Does
> it
> > override DidChangeBounds deliberately to avoid this?
> 
> Removing Layout call.
> 
> http://codereview.chromium.org/151150/diff/1/2#newcode1542
> Line 1542: contents_split_->Layout();
> On 2009/07/01 19:43:39, sky wrote:
> > On 2009/07/01 18:20:08, Ben Goodger wrote:
> > > Shouldn't the split view be laid out automatically as its bounds change?
> Does
> > it
> > > override DidChangeBounds deliberately to avoid this?
> > 
> > SingleSplitView doesn't do anything special, so as you say, the Layout call
> > shouldn't be needed.
> 
> Done
> 
> http://codereview.chromium.org/151150/diff/1/2#newcode1585
> Line 1585: void BrowserView::UpdateDevToolsForContents(TabContents*
> tab_contents) {
> On 2009/07/01 18:20:08, Ben Goodger wrote:
> > function order in CC should ALWAYS MATCH THE HEADER.
> 
> Done.
> 
> http://codereview.chromium.org/151150/diff/1/3
> File chrome/browser/views/frame/browser_view.h (right):
> 
> http://codereview.chromium.org/151150/diff/1/3#newcode361
> Line 361: void UpdateDevToolsForContents(TabContents* tab_contents);
> On 2009/07/01 18:20:08, Ben Goodger wrote:
> > Move this method up to below MaybeShowInfoBar.
> 
> Done.
> 
> http://codereview.chromium.org/151150/diff/1/3#newcode416
> Line 416: // Split view containing contents container and devtools container.
> On 2009/07/01 18:20:08, Ben Goodger wrote:
> > containing _the_
> 
> Done.

LGTM

LGTM

Expand Messages | Collapse Messages