Move download shelf from per-tab to per-window

Move download shelf from per-tab to per-window.  Also disable auto-hiding of
the shelf.

BUG=9025
TEST=Download file in one tab, open new tab, and check that download shelf is
still open. Also try the shelf's close button and the "show all downloads"
link. When saving a file, the download animation should not show up.

[Nico, 2009-05-26 17:15:37]

I haven't tested the windows changes yet, due to lack of a windows machine. It
builds and passes all tests, but that doesn't say much :-) It seems to work on
linux.

paulg, you're the main reviewer.

estade, you're here for the linux bits. I'm not very familiar with GTK.

[Evan Stade, 2009-05-26 17:41:49]

cool.

http://codereview.chromium.org/115740/diff/1795/689
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/115740/diff/1795/689#newcode154
Line 154: virtual DownloadShelf* GetDownloadShelf(bool create) = 0;
I suspect that the parameter is no longer necessary and all callers pass true.

http://codereview.chromium.org/115740/diff/1795/707
File chrome/browser/download/download_shelf.h (right):

http://codereview.chromium.org/115740/diff/1795/707#newcode30
Line 30: static DownloadShelf* Create(Browser* browser, gfx::NativeView view);
I think you can get rid of this factory method now. It only existed because the
download shelf was being created in a platform-agnostic bit of code, but now it
isn't.

http://codereview.chromium.org/115740/diff/1795/699
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/699#newcode582
Line 582: download_shelf_.reset(DownloadShelf::Create(browser_.get(),
window_vbox_));
|render_area_vbox_|

http://codereview.chromium.org/115740/diff/1795/699#newcode624
Line 624: // The download shelf is controlled by its TabContents, so we don't
have to
this function needs updating (it should add the height of the download shelf if
it's closing)

http://codereview.chromium.org/115740/diff/1795/701
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/701#newcode169
Line 169: body_.Own(gtk_button_new());
this doesn't need to be an OwnedWidget

(We usually only use OwnedWidgets for one widget per c++ class, and it's the
highest-level widget)

http://codereview.chromium.org/115740/diff/1795/701#newcode211
Line 211: progress_area_.Own(gtk_fixed_new());
this doesn't need to be an OwnedWidget

http://codereview.chromium.org/115740/diff/1795/701#newcode260
Line 260: if (g_signal_handler_find(parent_shelf_->GetHBox(),
so when do download items get destroyed now? how is it different from before
that we now need this extra check?

http://codereview.chromium.org/115740/diff/1795/695
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/695#newcode75
Line 75: hbox_.Own(gtk_hbox_new(FALSE, kDownloadItemPadding));
this doesn't need to be OwnedWidget

http://codereview.chromium.org/115740/diff/1795/695#newcode162
Line 162: if (browser_ && browser_->window())
I don't think you need this check (here and elsewhere). Wouldn't it be invalid
for browser_ to be NULL? And in DownloadShelf you use brower_ without checking
it.

Same for browser_->window(). There are plenty of places in the code we don't
check browser_->window().

[Evan Stade, 2009-05-26 18:24:16]

come to think of it, you can get rid of vbox_ in TabContentsViewGtk now. After
this change it will only contain fixed_, so just replace all its uses with
fixed_ (and make fixed_ an OwnedWidgetGtk).

[Nico, 2009-05-26 21:58:59]

On 2009/05/26 18:24:16, Evan Stade wrote:
> come to think of it, you can get rid of vbox_ in TabContentsViewGtk now. After
> this change it will only contain fixed_, so just replace all its uses with
> fixed_ (and make fixed_ an OwnedWidgetGtk).

Done. Also addressed most other comments, only OwnedGtkWidget still doesn't work
as I (and you too) expect.

[Nico, 2009-05-26 22:00:27]

Forgot to send my comments with the last mail.

http://codereview.chromium.org/115740/diff/1795/689
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/115740/diff/1795/689#newcode154
Line 154: virtual DownloadShelf* GetDownloadShelf(bool create) = 0;
On 2009/05/26 17:41:49, Evan Stade wrote:
> I suspect that the parameter is no longer necessary and all callers pass true.

Done.

http://codereview.chromium.org/115740/diff/1795/707
File chrome/browser/download/download_shelf.h (right):

http://codereview.chromium.org/115740/diff/1795/707#newcode30
Line 30: static DownloadShelf* Create(Browser* browser, gfx::NativeView view);
On 2009/05/26 17:41:49, Evan Stade wrote:
> I think you can get rid of this factory method now. It only existed because
the
> download shelf was being created in a platform-agnostic bit of code, but now
it
> isn't.

Done.

http://codereview.chromium.org/115740/diff/1795/699
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/699#newcode582
Line 582: download_shelf_.reset(DownloadShelf::Create(browser_.get(),
window_vbox_));
On 2009/05/26 17:41:49, Evan Stade wrote:
> |render_area_vbox_|
> 
> 

Done.

http://codereview.chromium.org/115740/diff/1795/699#newcode624
Line 624: // The download shelf is controlled by its TabContents, so we don't
have to
On 2009/05/26 17:41:49, Evan Stade wrote:
> this function needs updating (it should add the height of the download shelf
if
> it's closing)

Done, I think.

http://codereview.chromium.org/115740/diff/1795/701
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/701#newcode169
Line 169: body_.Own(gtk_button_new());
On 2009/05/26 17:41:49, Evan Stade wrote:
> this doesn't need to be an OwnedWidget
> 
> (We usually only use OwnedWidgets for one widget per c++ class, and it's the
> highest-level widget)

I originally had only hbox_ as owned widget, but then I got assertions when
closing the chrome window.

Here's why the 3 OwnedWidgets are there:

* body_: AnimationProgressed() is still called after the browser window widget
has been gtk_widget_destroy()ed. Without this, gtk asserts that GTK_IS_WIDGET
fails in gtk_widget_set_size(body_). Doesn't happen with the owned widget.

* progress_area_: Same thing in AnimationProgressed()'s other branch
(gtk_widget_queue_draw()).

* DownloadShelfGtk::hbox_: Would in ~DownloadItemGtk, which calls
parent_shelf_->GetHBox().

I just tried changing body_ and progress_area_ back to regular pointers, but I
still get the same asserts I got yesterday.

http://codereview.chromium.org/115740/diff/1795/701#newcode260
Line 260: if (g_signal_handler_find(parent_shelf_->GetHBox(),
On 2009/05/26 17:41:49, Evan Stade wrote:
> so when do download items get destroyed now? how is it different from before
> that we now need this extra check?

They are destroyed when the shelf is destroyed, which happens when
~BrowserWindowGtk() runs. The gtk window widget is already destroyed in
BrowserWindowGtk::Close(), so the items now die after the browser window widget
is gone.

http://codereview.chromium.org/115740/diff/1795/695
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/695#newcode162
Line 162: if (browser_ && browser_->window())
On 2009/05/26 17:41:49, Evan Stade wrote:
> I don't think you need this check (here and elsewhere). Wouldn't it be invalid
> for browser_ to be NULL? And in DownloadShelf you use brower_ without checking
> it.
> 
> Same for browser_->window(). There are plenty of places in the code we don't
> check browser_->window().

Done. Added a DCHECK to DownloadShelf's constructor for good measure.

[Evan Stade, 2009-05-26 22:00:57]

On 2009/05/26 21:58:59, thakis wrote:
> On 2009/05/26 18:24:16, Evan Stade wrote:
> > come to think of it, you can get rid of vbox_ in TabContentsViewGtk now.
After
> > this change it will only contain fixed_, so just replace all its uses with
> > fixed_ (and make fixed_ an OwnedWidgetGtk).
> 
> Done. Also addressed most other comments, only OwnedGtkWidget still doesn't
work
> as I (and you too) expect.

Sorry? how do you mean?

[Evan Stade, 2009-05-26 22:09:33]

* body_: AnimationProgressed() is still called after the browser window widget
has been gtk_widget_destroy()ed. Without this, gtk asserts that GTK_IS_WIDGET
fails in gtk_widget_set_size(body_). Doesn't happen with the owned widget.

* progress_area_: Same thing in AnimationProgressed()'s other branch
(gtk_widget_queue_draw()).

ah, well I see how that would fix it, but it doesn't sound like the right fix.
Why are we getting AnimationProgressed() calls on a c++ object that's already
been destructed?

* DownloadShelfGtk::hbox_: Would in ~DownloadItemGtk, which calls
parent_shelf_->GetHBox().

The hbox will not be destroyed till its ancestor is destroyed (shelf_ in this
case). We should be safe because the DownloadItemGtk is destroyed in the
DownloadShelfGtk d'tor, but before shelf_.Destroy(). If a download item is
getting destroyed after that point, it is an error.

[Evan Stade, 2009-05-27 03:02:12]

http://codereview.chromium.org/115740/diff/1795/701
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1795/701#newcode260
Line 260: if (g_signal_handler_find(parent_shelf_->GetHBox(),
On 2009/05/26 22:00:27, thakis wrote:
> On 2009/05/26 17:41:49, Evan Stade wrote:
> > so when do download items get destroyed now? how is it different from before
> > that we now need this extra check?
> 
> They are destroyed when the shelf is destroyed, which happens when
> ~BrowserWindowGtk() runs. The gtk window widget is already destroyed in
> BrowserWindowGtk::Close(), so the items now die after the browser window
widget
> is gone.

ok, I think this is related to the destroy thing (the signal handler should
remove itself when the widget gets the "destroy" signal). It won't be necessary
if Dean agrees to that patch I made, but it's fine for now.

http://codereview.chromium.org/115740/diff/887/888
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/115740/diff/887/888#newcode150
Line 150: virtual bool IsDownloadShelfVisible() = 0;
const

http://codereview.chromium.org/115740/diff/887/898
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/887/898#newcode577
Line 577: return shelf_visible_;
how about

  return download_shelf_.get() && download_shelf_->IsShowing()

and get rid of shelf_visible_

http://codereview.chromium.org/115740/diff/887/898#newcode627
Line 627: if (const_cast<BrowserWindowGtk*>(this)->IsDownloadShelfVisible()) {
should be:

if (download_shelf_.get() && download_shelf_->IsClosing())

[Nico, 2009-05-27 04:55:36]

All done.

I also tested this on windows, and thinks seem to work on windows too by now.

http://codereview.chromium.org/115740/diff/887/888
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/115740/diff/887/888#newcode150
Line 150: virtual bool IsDownloadShelfVisible() = 0;
On 2009/05/27 03:02:12, Evan Stade wrote:
> const

Done.

http://codereview.chromium.org/115740/diff/887/898
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/887/898#newcode577
Line 577: return shelf_visible_;
On 2009/05/27 03:02:12, Evan Stade wrote:
> how about
> 
>   return download_shelf_.get() && download_shelf_->IsShowing()
> 
> and get rid of shelf_visible_

Done.

http://codereview.chromium.org/115740/diff/887/898#newcode627
Line 627: if (const_cast<BrowserWindowGtk*>(this)->IsDownloadShelfVisible()) {
On 2009/05/27 03:02:12, Evan Stade wrote:
> should be:
> 
> if (download_shelf_.get() && download_shelf_->IsClosing())

Done.

[Nico, 2009-05-27 04:55:56]

All done.

I also tested this on windows, and things seem to work there too by now.

[Evan Stade, 2009-05-27 17:22:00]

http://codereview.chromium.org/115740/diff/1948/1959
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1948/1959#newcode569
Line 569: GetDownloadShelf();
This function is seeming kind of pointless now, especially since it doesn't
actually ever do what its name purports to do (except if the download shelf has
not been made and visible is true). Can we chuck it?

http://codereview.chromium.org/115740/diff/1948/1955
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1948/1955#newcode148
Line 148: return GetHBox()->allocation.height;
return slide_widget_->widget()->allocation.height;

http://codereview.chromium.org/115740/diff/1948/1955#newcode160
Line 160: browser_->window()->SetDownloadShelfVisible(false);
no longer needed

http://codereview.chromium.org/115740/diff/1948/1955#newcode177
Line 177: shelf->browser_->window()->SetDownloadShelfVisible(false);
no longer needed

http://codereview.chromium.org/115740/diff/1948/1972
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/115740/diff/1948/1972#newcode829
Line 829: // SetShelfVisible can force-close the shelf, so make sure we lay out
SetShelfVisible -> SetDownloadShelfVisible (that typo is probably my fault
actually)

http://codereview.chromium.org/115740/diff/1948/1972#newcode832
Line 832: SelectedTabToolbarSizeChanged(false);
What does this do exactly? Aside from this line, it seems like we could just
chuck SetDownloadShelfVisible completely. If this line is really important, can
we just call it directly from any place that's currently using
SetDownloadShelfVisible(false) and call GetDownloadShelf() from any place that's
currently using SetDownloadShelfVisible(true)

(on a side note, it seems that the name of the function is somewhat
misleading---it only actually affects the showing state of the shelf if the
shelf has not been made and |visible| is true, right?)

http://codereview.chromium.org/115740/diff/1948/1972#newcode835
Line 835: bool BrowserView::IsDownloadShelfVisible() const {
do something similar to gtk?

http://codereview.chromium.org/115740/diff/7015/4074
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/115740/diff/7015/4074#newcode1165
Line 1165: window()->SetDownloadShelfVisible(true);
necessary?

[Nico, 2009-05-29 22:24:20]

Was busy with work the last day, sorry bout that.

http://codereview.chromium.org/115740/diff/1948/1959
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1948/1959#newcode569
Line 569: GetDownloadShelf();
On 2009/05/27 17:22:00, Evan Stade wrote:
> This function is seeming kind of pointless now, especially since it doesn't
> actually ever do what its name purports to do (except if the download shelf
has
> not been made and visible is true). Can we chuck it?

Chucked it for GTK. I'd prefer not chucking it in windows, because the current
way works in windows, doing the obvious edits breaks it, and I do my windows dev
in a vm, which is very slow, so I'd rather not invest time in that now.

I did remove the function from browser_window.h though.

http://codereview.chromium.org/115740/diff/1948/1955
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1948/1955#newcode148
Line 148: return GetHBox()->allocation.height;
On 2009/05/27 17:22:00, Evan Stade wrote:
> return slide_widget_->widget()->allocation.height;
> 

Done.

http://codereview.chromium.org/115740/diff/1948/1955#newcode160
Line 160: browser_->window()->SetDownloadShelfVisible(false);
On 2009/05/27 17:22:00, Evan Stade wrote:
> no longer needed

Put something else here that was in SetDSVisible() after merging this morning
(this ends up notifying the status bubble about the shelf).

http://codereview.chromium.org/115740/diff/1948/1972
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/115740/diff/1948/1972#newcode829
Line 829: // SetShelfVisible can force-close the shelf, so make sure we lay out
On 2009/05/27 17:22:00, Evan Stade wrote:
> SetShelfVisible -> SetDownloadShelfVisible (that typo is probably my fault
> actually)

Done.

http://codereview.chromium.org/115740/diff/1948/1972#newcode832
Line 832: SelectedTabToolbarSizeChanged(false);
On 2009/05/27 17:22:00, Evan Stade wrote:
> What does this do exactly? Aside from this line, it seems like we could just
> chuck SetDownloadShelfVisible completely. If this line is really important,
can
> we just call it directly from any place that's currently using
> SetDownloadShelfVisible(false) and call GetDownloadShelf() from any place
that's
> currently using SetDownloadShelfVisible(true)
> 
> (on a side note, it seems that the name of the function is somewhat
> misleading---it only actually affects the showing state of the shelf if the
> shelf has not been made and |visible| is true, right?)

It ends up calling Layout() (through UpdateUIForContents), which tells the shelf
object if it should be visible. WIthout this, the shelf would sometimes not show
up (if the computer is slow enough that no shelf slide animation frames are
dsplayed at all – happens all the time in a VM for example). This is also how
this was done before.

Yes, the name is misleading, the funciton only updates the internal show state.

After merging, this funciton does slightly more.

http://codereview.chromium.org/115740/diff/1948/1972#newcode835
Line 835: bool BrowserView::IsDownloadShelfVisible() const {
On 2009/05/27 17:22:00, Evan Stade wrote:
> do something similar to gtk?

Done.

http://codereview.chromium.org/115740/diff/7015/4074
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/115740/diff/7015/4074#newcode1165
Line 1165: window()->SetDownloadShelfVisible(true);
On 2009/05/27 17:22:01, Evan Stade wrote:
> necessary?

No, nixed it.

[Nico, 2009-05-29 22:25:52]

As touched on in a comment, I'd rather leave the windows side as is if you don't
find glaring obmissions. It could probably cleaned up some more, but not much,
and running msvc in a vm is really really slow over here.

[Evan Stade, 2009-05-29 22:48:43]

lgtm for linux.

http://codereview.chromium.org/115740/diff/1948/1959
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/115740/diff/1948/1959#newcode569
Line 569: GetDownloadShelf();
On 2009/05/29 22:24:20, thakis wrote:
> On 2009/05/27 17:22:00, Evan Stade wrote:
> > This function is seeming kind of pointless now, especially since it doesn't
> > actually ever do what its name purports to do (except if the download shelf
> has
> > not been made and visible is true). Can we chuck it?
> 
> Chucked it for GTK. I'd prefer not chucking it in windows, because the current
> way works in windows, doing the obvious edits breaks it, and I do my windows
dev
> in a vm, which is very slow, so I'd rather not invest time in that now.
> 
> I did remove the function from browser_window.h though.

good enough for me :)

http://codereview.chromium.org/115740/diff/7114/7121
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/7114/7121#newcode161
Line 161: browser_->UpdateDownloadShelfVisibility(false);
ah I see. This is super annoying. I think the status bubble ought to know about
its browser window, and query that for visibility rather than having its own
member variable that we are responsible for updating.

That can be another patch though. Can you put a TODO for me to clean this up?

[Nico, 2009-05-29 22:51:38]

Thanks!

http://codereview.chromium.org/115740/diff/7114/7121
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/115740/diff/7114/7121#newcode161
Line 161: browser_->UpdateDownloadShelfVisibility(false);
On 2009/05/29 22:48:43, Evan Stade wrote:
> ah I see. This is super annoying. I think the status bubble ought to know
about
> its browser window, and query that for visibility rather than having its own
> member variable that we are responsible for updating.
> 
> That can be another patch though. Can you put a TODO for me to clean this up?

Put in a TODO for you.

[Paul Godavari, 2009-06-02 01:26:59]

When I patched your change locally and tested it, I found a problem doing the
following:
1. Download something.
2. Open a new tab (shelf is visible here).
3. In that new tab, close the shelf.
4. Go back to the first tab.

In the first tab, the shelf is still visible and can't be closed. Is there a
ui_test that you could add for this?

http://codereview.chromium.org/115740/diff/4402/4416
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/115740/diff/4402/4416#newcode1173
Line 1173: if (platform_util::IsVisible(current_tab->GetNativeView())) {
Nit: don't need the braces for 1 line (also to be consistent with earlier if).

http://codereview.chromium.org/115740/diff/4402/4420
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/115740/diff/4402/4420#newcode14
Line 14: #include "chrome/browser/tab_contents/tab_contents.h"
Is tab_contents.h still needed?

http://codereview.chromium.org/115740/diff/4402/4421
File chrome/browser/download/download_shelf.h (right):

http://codereview.chromium.org/115740/diff/4402/4421#newcode16
Line 16: class TabContents;
Do you still need to declare TabContents?

http://codereview.chromium.org/115740/diff/4402/4441
File chrome/browser/gtk/download_item_gtk.h (right):

http://codereview.chromium.org/115740/diff/4402/4441#newcode117
Line 117: //GtkWidget* body_;
Nit: Space after //

http://codereview.chromium.org/115740/diff/4402/4441#newcode134
Line 134: //GtkWidget* progress_area_;
Nit: Space after //

http://codereview.chromium.org/115740/diff/4402/4412
File chrome/browser/gtk/download_shelf_gtk.h (right):

http://codereview.chromium.org/115740/diff/4402/4412#newcode54
Line 54: //GtkWidget* hbox_;
Nit: Space.

http://codereview.chromium.org/115740/diff/4402/4406
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/115740/diff/4402/4406#newcode928
Line 928: if (tab_contents && tab_contents->delegate()) {
Nit: Don't need braces here.

http://codereview.chromium.org/115740/diff/4402/4426
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/115740/diff/4402/4426#newcode279
Line 279: download_shelf_(NULL),
This needs to be in the same order as listed in the header.

http://codereview.chromium.org/115740/diff/4402/4443
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/115740/diff/4402/4443#newcode440
Line 440: virtual bool IsClosing() const { return false; }
These methods should all call NOTIMPLEMENTED().

http://codereview.chromium.org/115740/diff/4402/4431
File chrome/test/test_browser_window.h (right):

http://codereview.chromium.org/115740/diff/4402/4431#newcode56
Line 56: virtual DownloadShelf* GetDownloadShelf() { return 0; }
Use NULL.

[Nico, 2009-06-02 16:44:06]

Hi Paul,

thanks for the review. I've addressed all comments.

I tested the "open shelf, open tab, close shelf, switch to first tab" scenario
and can't see the problem locally (but my local build updates at 2 fps, so
animation-related stuff might work differently on your box). I also added a test
(ui_tests --gtest_filter=DownloadTest.PerWindowShelf) that passes on the windows
try bots. Can you reproduce this every time? Do you wait until the shelf is
fully visible before closing it? Can you give more detailed instructions on how
to reproduce this?

http://codereview.chromium.org/115740/diff/4402/4416
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/115740/diff/4402/4416#newcode1173
Line 1173: if (platform_util::IsVisible(current_tab->GetNativeView())) {
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Nit: don't need the braces for 1 line (also to be consistent with earlier if).

Done.

http://codereview.chromium.org/115740/diff/4402/4420
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/115740/diff/4402/4420#newcode14
Line 14: #include "chrome/browser/tab_contents/tab_contents.h"
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Is tab_contents.h still needed?

Done.

http://codereview.chromium.org/115740/diff/4402/4421
File chrome/browser/download/download_shelf.h (right):

http://codereview.chromium.org/115740/diff/4402/4421#newcode16
Line 16: class TabContents;
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Do you still need to declare TabContents?

Done.

http://codereview.chromium.org/115740/diff/4402/4441
File chrome/browser/gtk/download_item_gtk.h (right):

http://codereview.chromium.org/115740/diff/4402/4441#newcode117
Line 117: //GtkWidget* body_;
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Nit: Space after //

Done.

http://codereview.chromium.org/115740/diff/4402/4441#newcode134
Line 134: //GtkWidget* progress_area_;
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Nit: Space after //

Done.

http://codereview.chromium.org/115740/diff/4402/4412
File chrome/browser/gtk/download_shelf_gtk.h (right):

http://codereview.chromium.org/115740/diff/4402/4412#newcode54
Line 54: //GtkWidget* hbox_;
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Nit: Space.

Done.

http://codereview.chromium.org/115740/diff/4402/4406
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/115740/diff/4402/4406#newcode928
Line 928: if (tab_contents && tab_contents->delegate()) {
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Nit: Don't need braces here.

Done.

http://codereview.chromium.org/115740/diff/4402/4426
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/115740/diff/4402/4426#newcode279
Line 279: download_shelf_(NULL),
On 2009/06/02 01:26:59, Paul Godavari wrote:
> This needs to be in the same order as listed in the header.

What was I thinking. Done.

http://codereview.chromium.org/115740/diff/4402/4443
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/115740/diff/4402/4443#newcode440
Line 440: virtual bool IsClosing() const { return false; }
On 2009/06/02 01:26:59, Paul Godavari wrote:
> These methods should all call NOTIMPLEMENTED().

Done.

http://codereview.chromium.org/115740/diff/4402/4431
File chrome/test/test_browser_window.h (right):

http://codereview.chromium.org/115740/diff/4402/4431#newcode56
Line 56: virtual DownloadShelf* GetDownloadShelf() { return 0; }
On 2009/06/02 01:26:59, Paul Godavari wrote:
> Use NULL.

Done.

[Paul Godavari, 2009-06-04 01:19:33]

LGTM with nits fixed.

http://codereview.chromium.org/115740/diff/7416/7418
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/115740/diff/7416/7418#newcode1960
Line 1960: //
Nit: remove blank comment.

http://codereview.chromium.org/115740/diff/7416/7418#newcode1971
Line 1971: //
Nit: remove blank comment.

http://codereview.chromium.org/115740/diff/7416/7452
File chrome/browser/download/download_uitest.cc (right):

http://codereview.chromium.org/115740/diff/7416/7452#newcode228
Line 228: TEST_F(DownloadTest, PerWindowShelf) {
Thanks for adding this test.

http://codereview.chromium.org/115740/diff/7416/7420
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/115740/diff/7416/7420#newcode924
Line 924: 
Nit: remove extra blank line.

[Nico, 2009-06-04 01:26:18]

Thanks!

I will land this when the tree is open again.

http://codereview.chromium.org/115740/diff/7416/7418
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/115740/diff/7416/7418#newcode1960
Line 1960: //
On 2009/06/04 01:19:33, Paul Godavari wrote:
> Nit: remove blank comment.

Done.

http://codereview.chromium.org/115740/diff/7416/7418#newcode1971
Line 1971: //
On 2009/06/04 01:19:33, Paul Godavari wrote:
> Nit: remove blank comment.

Done.

http://codereview.chromium.org/115740/diff/7416/7420
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/115740/diff/7416/7420#newcode924
Line 924: 
On 2009/06/04 01:19:33, Paul Godavari wrote:
> Nit: remove extra blank line.

Done.