DevTools: Initial commit with docked devtools window support on Linux.

DevTools: Initial commit with docked devtools window support on Linux.


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

[pfeldman, 2009-07-15 17:42:39]

Hi guys, could you take a look at this? Comments, suggestions are welcome!

Pavel

[tony, 2009-07-15 17:58:47]

http://codereview.chromium.org/149675/diff/1005/24
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/24#newcode923
Line 923: bool should_show = old_devtools == NULL && devtools_contents != NULL;
This seems overly complex.  ShowContainer should be safe to call if we're
already showing the container.  I would probably just call ShowContainer if
devtools_contents != NULL and call HideContainer if devtools_contents == NULL.

http://codereview.chromium.org/149675/diff/1005/24#newcode1134
Line 1134: gtk_paned_set_position(GTK_PANED(contents_split_), 400);
Is 400 the distance from the top?  Shouldn't it be based on the distance from
the bottom?  Can you make this to a constant?

http://codereview.chromium.org/149675/diff/1005/22
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/22#newcode187
Line 187: GtkWidget* widget = tab_contents_container->status_bubble_->widget();
Do we need to check for a NULL status_bubble_ here or does this not get called
for the dev tools window?

http://codereview.chromium.org/149675/diff/1005/23
File chrome/browser/gtk/tab_contents_container_gtk.h (right):

http://codereview.chromium.org/149675/diff/1005/23#newcode33
Line 33: // Inserts our GtkWidget* hierarchy into a GtkPaned managed by our
owner.
I realize we do this in other places, but I think it would be simpler if we just
added GtkWidget* widget() { return floating_->widget(); } to this class and did
the gtk_paned_add* calls in browser_window_gtk.cc.

[pfeldman, 2009-07-15 18:24:50]

Thanks for your review. There are couple of questions inside, would appreciate
your hints!

http://codereview.chromium.org/149675/diff/1005/24
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/24#newcode923
Line 923: bool should_show = old_devtools == NULL && devtools_contents != NULL;
On 2009/07/15 17:58:47, tony wrote:
> This seems overly complex.  ShowContainer should be safe to call if we're
> already showing the container.  I would probably just call ShowContainer if
> devtools_contents != NULL and call HideContainer if devtools_contents == NULL.

I was trying to mimic browser_view's code. These two flags are used there to
manage focus properly - not sure whether this will be needed in gtk. If you
generally choose simplicity over windows-alike structure, I am ready to simplify
this.

http://codereview.chromium.org/149675/diff/1005/24#newcode1134
Line 1134: gtk_paned_set_position(GTK_PANED(contents_split_), 400);
On 2009/07/15 17:58:47, tony wrote:
> Is 400 the distance from the top?  Shouldn't it be based on the distance from
> the bottom?  Can you make this to a constant?

Ideally it should be a constant offset from the bottom, but I am not sure where
I can get adequate contents_split_ size in the first place. Any hints?

http://codereview.chromium.org/149675/diff/1005/22
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/22#newcode187
Line 187: GtkWidget* widget = tab_contents_container->status_bubble_->widget();
On 2009/07/15 17:58:47, tony wrote:
> Do we need to check for a NULL status_bubble_ here or does this not get called
> for the dev tools window?

We are not registering this method as a callback above, so I was assuming
OnSetFloatingPosition will never trigger for this secondary container.

http://codereview.chromium.org/149675/diff/1005/23
File chrome/browser/gtk/tab_contents_container_gtk.h (right):

http://codereview.chromium.org/149675/diff/1005/23#newcode33
Line 33: // Inserts our GtkWidget* hierarchy into a GtkPaned managed by our
owner.
On 2009/07/15 17:58:47, tony wrote:
> I realize we do this in other places, but I think it would be simpler if we
just
> added GtkWidget* widget() { return floating_->widget(); } to this class and
did
> the gtk_paned_add* calls in browser_window_gtk.cc.

Done.

[tony, 2009-07-15 18:46:59]

On 2009/07/15 18:24:50, pfeldman wrote:
> http://codereview.chromium.org/149675/diff/1005/24
> File chrome/browser/gtk/browser_window_gtk.cc (right):
> 
> http://codereview.chromium.org/149675/diff/1005/24#newcode923
> Line 923: bool should_show = old_devtools == NULL && devtools_contents !=
NULL;
> On 2009/07/15 17:58:47, tony wrote:
> > This seems overly complex.  ShowContainer should be safe to call if we're
> > already showing the container.  I would probably just call ShowContainer if
> > devtools_contents != NULL and call HideContainer if devtools_contents ==
NULL.
> 
> I was trying to mimic browser_view's code. These two flags are used there to
> manage focus properly - not sure whether this will be needed in gtk. If you
> generally choose simplicity over windows-alike structure, I am ready to
simplify
> this.

I don't have a strong opinion about this, this is fine.


> http://codereview.chromium.org/149675/diff/1005/24#newcode1134
> Line 1134: gtk_paned_set_position(GTK_PANED(contents_split_), 400);
> On 2009/07/15 17:58:47, tony wrote:
> > Is 400 the distance from the top?  Shouldn't it be based on the distance
from
> > the bottom?  Can you make this to a constant?
> 
> Ideally it should be a constant offset from the bottom, but I am not sure
where
> I can get adequate contents_split_ size in the first place. Any hints?

The way to do this in GTK+ is to hook up a size-request signal to the tab
contents container widget and request that that height be some number of pixels.
 It's kind of a pain since widgets size based on the size of their children.

I would just leave it as 400 for now, file a bug that it should be sized from
the bottom, and add a comment to the code pointing to the bug.  It's a big
enough change that we can punt for now and do it in a follow up change.

LGTM!

[Evan Stade, 2009-07-15 18:55:01]

lgtm with nit

http://codereview.chromium.org/149675/diff/1005/24
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/24#newcode923
Line 923: bool should_show = old_devtools == NULL && devtools_contents != NULL;
On 2009/07/15 18:24:50, pfeldman wrote:
> On 2009/07/15 17:58:47, tony wrote:
> > This seems overly complex.  ShowContainer should be safe to call if we're
> > already showing the container.  I would probably just call ShowContainer if
> > devtools_contents != NULL and call HideContainer if devtools_contents ==
NULL.
> 
> I was trying to mimic browser_view's code. These two flags are used there to
> manage focus properly - not sure whether this will be needed in gtk. If you
> generally choose simplicity over windows-alike structure, I am ready to
simplify
> this.

we don't match windows overly much in views code, particularly not in focus
management. So please do simplify it.

[pfeldman, 2009-07-16 09:20:26]

http://codereview.chromium.org/149675/diff/1005/24
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/149675/diff/1005/24#newcode923
Line 923: bool should_show = old_devtools == NULL && devtools_contents != NULL;
On 2009/07/15 18:55:01, Evan Stade wrote:
> On 2009/07/15 18:24:50, pfeldman wrote:
> > On 2009/07/15 17:58:47, tony wrote:
> > > This seems overly complex.  ShowContainer should be safe to call if we're
> > > already showing the container.  I would probably just call ShowContainer
if
> > > devtools_contents != NULL and call HideContainer if devtools_contents ==
> NULL.
> > 
> > I was trying to mimic browser_view's code. These two flags are used there to
> > manage focus properly - not sure whether this will be needed in gtk. If you
> > generally choose simplicity over windows-alike structure, I am ready to
> simplify
> > this.
> 
> we don't match windows overly much in views code, particularly not in focus
> management. So please do simplify it.

Done.

[Evan Stade, 2009-07-16 17:57:08]

thanks, lgtm