media player: list management.

media player list management.


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

[fbarchard, 2009-04-27 22:49:42]

Can you review this one?  Its fairly small

[awong, 2009-04-27 23:05:40]

http://codereview.chromium.org/100078/diff/1/2
File media/player/list.h (right):

http://codereview.chromium.org/100078/diff/1/2#newcode5
Line 5: // list.h : interface of the CMruList class
Can you describe what is the expected use of the CMruList class?

Also, this isn't the interface, it's the fully implementation.

http://codereview.chromium.org/100078/diff/1/2#newcode13
Line 13: SIZE m_size;
Does m_size need to be public?

http://codereview.chromium.org/100078/diff/1/2#newcode23
Line 23: WS_VSCROLL | LBS_NOINTEGRALHEIGHT,
This file is missing #includes, making it dependent on inclusion order.  Can we
#include everything it needs?

http://codereview.chromium.org/100078/diff/1/2#newcode37
Line 37: InsertString(0, mru.m_arrDocs[i].szDocName);  // Reverse order in
array.
dont' understand this code completely...are we calling a parent classe's insert?
 This isn't going to N^2 is it?

http://codereview.chromium.org/100078/diff/1/2#newcode39
Line 39: if (nSize > 0) {
What happens if nSize == 0?

http://codereview.chromium.org/100078/diff/1/2#newcode63
Line 63: ::SetFocus(GetParent());  // will hide this window
Capitalization and punctuation in comment.

http://codereview.chromium.org/100078/diff/1/2#newcode77
Line 77: LRESULT OnKeyDown(UINT /*uMsg*/,
We're still doing that comment out unused parameter thing...I haven't seen in
any other code.  Can we uncomment these here and elsewhere?

[fbarchard, 2009-04-28 01:44:35]

done

http://codereview.chromium.org/100078/diff/1/2
File media/player/list.h (right):

http://codereview.chromium.org/100078/diff/1/2#newcode5
Line 5: // list.h : interface of the CMruList class
Note: This fail is asis from Microsoft.
Done.

http://codereview.chromium.org/100078/diff/1/2#newcode13
Line 13: SIZE m_size;
nope.  fixed

http://codereview.chromium.org/100078/diff/1/2#newcode23
Line 23: WS_VSCROLL | LBS_NOINTEGRALHEIGHT,
none of the WTL headers include their dependencies.  Are you sure we should do
this?

http://codereview.chromium.org/100078/diff/1/2#newcode37
Line 37: InsertString(0, mru.m_arrDocs[i].szDocName);  // Reverse order in
array.
Looks like O(N^2) yes.

http://codereview.chromium.org/100078/diff/1/2#newcode39
Line 39: if (nSize > 0) {
The list is not valid... it won't be clickable.

http://codereview.chromium.org/100078/diff/1/2#newcode63
Line 63: ::SetFocus(GetParent());  // will hide this window
On 2009/04/27 23:05:41, awong wrote:
> Capitalization and punctuation in comment.

Done.

http://codereview.chromium.org/100078/diff/1/2#newcode77
Line 77: LRESULT OnKeyDown(UINT /*uMsg*/,
This is to avoid warnings on unused parameters, while documenting what the
parameters are.

[fbarchard, 2009-04-28 17:32:30]

looks good to me.  does it look good to you? :)

[awong, 2009-04-28 23:28:59]

LGTM

Please add a TODO at the top of the file saying we need to do a hungarain
cleanup.