Classification of TODOs in Panel code.

Classification of TODOs in Panel code.
Created some bugs, removed some TODOs.


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

[Dmitry Titov, 2011-11-02 22:15:01]

First pass. Covered all files in panels dir except panel_browsertest.cc.

Need LGTM from all 3 reviewers, added my comments where I did something other
then create a bug.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/base_p...
File chrome/browser/ui/panels/base_panel_browser_test.cc (left):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/base_p...
chrome/browser/ui/panels/base_panel_browser_test.cc:188: // these.
I removed this because a) it is of "if in the future" sort, and future is always
not as expected b) in tests, it's better to go for simplicity rather then code
reuse, because 'automatic feature pick up' in tests may be exactly not a good
thing (changes what is tested) and should be done rather explicitly.
In other words, this seems to be a not specific comment better suited for design
discussion then a TODO in code.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:237: // have starred state.
I took a liberty to 'resolve' it based on current understanding of Panels. Feel
free to object.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:252: // Size of panels is managed by
PanelManager, they are never 'zoomed'.
I took a liberty to 'resolve' it based on current understanding of Panels. Feel
free to object.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:294: // Panels don't have stop/reload
indicator.
I took a liberty to 'resolve' it based on current understanding of Panels. Feel
free to object.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (left):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:220: // TODO(saintlou):
This is dummy code, but a value must be returned.
That just states obvious thing.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:96: // TODO(jianli): Implement
for USE_AURA.
Didn't want a bug for this, maybe we shoudl just remove this because
NOTIMPLEMENTED() is enough?

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:435: if (!bring_up)
Who was I kidding? Having code for non-existing and not-understood conditions 
is never a good thing.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.h (left):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.h:79: // callsites with an
enum defined in PanelManager.
I imagined how this would look like and didn't like the idea of using
PanelManager enum in this file. Removed the TODO.

[jennb, 2011-11-02 22:25:31]

LGTM

Thanks for doing this cleanup.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:96: // TODO(jianli): Implement
for USE_AURA.
On 2011/11/02 22:15:01, Dmitry Titov wrote:
> Didn't want a bug for this, maybe we shoudl just remove this because
> NOTIMPLEMENTED() is enough?

Agree w/ removing TODO.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:160: NOTREACHED();  // View-based
Panels are not enabled anywhere else.
Can we delete the #else clause completely and convert the #elif into #else?

[jianli, 2011-11-02 22:55:01]

LGTM.

[prasadt, 2011-11-02 23:01:58]

Just one comment that needs addressed.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:433: // to combine them.
That sounds like a TODO that doesn't say TODO. Should be a TODO, bug or the line
should just be removed.

[jennb, 2011-11-02 23:10:21]

Still LGTM from me.

Good catch. Perhaps just change the comment to an explanation of why a
DCHECK is safe, e.g. no systems require both delays?

On Wed, Nov 2, 2011 at 4:01 PM, <prasadt@chromium.org> wrote:

> Just one comment that needs addressed.
>
>
>
> http://codereview.chromium.**org/8423060/diff/1/chrome/**
>
browser/ui/panels/panel_**manager.cc<http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_manager.cc>
> File chrome/browser/ui/panels/**panel_manager.cc (right):
>
> http://codereview.chromium.**org/8423060/diff/1/chrome/**
>
browser/ui/panels/panel_**manager.cc#newcode433<http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_manager.cc#newcode433>
> chrome/browser/ui/panels/**panel_manager.cc:433: // to combine them.
> That sounds like a TODO that doesn't say TODO. Should be a TODO, bug or
> the line should just be removed.
>
>
http://codereview.chromium.**org/8423060/<http://codereview.chromium.org/8423...
>

[Dmitry Titov, 2011-11-02 23:32:18]

Updated.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:96: // TODO(jianli): Implement
for USE_AURA.
On 2011/11/02 22:25:31, jennb wrote:
> On 2011/11/02 22:15:01, Dmitry Titov wrote:
> > Didn't want a bug for this, maybe we shoudl just remove this because
> > NOTIMPLEMENTED() is enough?
> 
> Agree w/ removing TODO.

Done.

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:160: NOTREACHED();  // View-based
Panels are not enabled anywhere else.
On 2011/11/02 22:25:31, jennb wrote:
> Can we delete the #else clause completely and convert the #elif into #else?

Done.
Made it consistent with the case in Deactivate(): 


#if defined(OS_WIN) && !defined(USE_AURA)
...
#else
  NOTIMPLEMENTED();
#endif

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8423060/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:433: // to combine them.
On 2011/11/02 23:01:58, prasadt wrote:
> That sounds like a TODO that doesn't say TODO. Should be a TODO, bug or the
line
> should just be removed.

I've reworded it, wanted an explanation for an assumption that DCHECK is
verifying.

[prasadt, 2011-11-03 01:06:15]

lgtm

[commit-bot: I haz the power, 2011-11-03 01:28:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/8423060/2002

[commit-bot: I haz the power, 2011-11-03 03:33:21]

Change committed as 108417