Initial checkin of media test matrix class, which will be used for media perf test later.

Initial checkin of media test matrix class, which will be used for media perf test later.

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

[Nirnimesh, 2011-03-17 02:31:59]

I'm OOO until next week, so I'm adding Dennis for reviewing this.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1:
"exts","bbee","mobile","bear","sync","color","scout3d","buck","gromit","ironman","trek","dartmoor","still","tulip","chicken","3dtv","avatar","avatar3d","wembly","pitivi","qjoe","birds","red","crowd","parkrun","sonyh","garden"
Add some comments to explain the format of this csv.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:10: VIDEO_TEST_MATRIX_HOME =
'http://tskir-html5.kir.corp.google.com/testmatrix/'
Do not reference internal URLs in public scripts. Please keep these in
pyauto_private in data dir.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:186: target: taget nickname
need a blank line before Returns:

Repeat for all the methods above

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:196: [url, nickname, tag] = l
You can use:

for url, nickname, tag in compact_list:
...

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:197: if(target == nickname):
remove parens

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:17: data_file =
os.path.join('..', 'data', 'media', 'media_matrix_data.csv')
use self.DataDir()

[dennis_jeffrey, 2011-03-17 17:13:36]

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:16: format.
The first line of this class docstring should be a 1-line summary, followed by a
blank line, followed by any  additional information.  I recommend you add some
more details here about what this class does and how to use it.  What is a
"video matrix file" that needs to be given as input?  What are the "different
formats" that are output?

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:20: # video specifications or video
descriptions.
Capitalize "v" in "video"

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:23: video_titles = []
Are the above lists meant to be public attributes?  If so, you need to list them
in an "Attributes:" section within the class docstring above.  If not, I believe
it's convention to name them starting with an underscore.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:27: variables.
This should be a 1-line summary of this method.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:34:
http://tskir-html5.kir.corp.google.com/testmatrix/index.html
As Nirnimesh said, we should not reference internal URLs in public scripts.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:37: audio files are stored more then
15th row
Capitalize the start of each sentence and put periods at the end.  Also, change
"then" to "than" here.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:40: above
Put period at the end of this sentence.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:44: for row in reader:
I think you can do:

for counter, row in enumerate(reader):

This will let you avoid having to initialize counter to 0 and having to
increment it on each loop iteration below.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:53:
self.specs[self.video_titles[i]].append(row[i + 1])
It might be good to do some kind of error checking here.  This line assumes that
"self.video_titles[i]" is defined, which is true only if the CSV file is
formatted according to your expectations.  If the CSV file is not formatted
properly, I think an exception could be raised here.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:61: name: the name of the video file
(column name in the CSV file)
Put periods at the end of each sentence in the above 2 lines.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:62: Returns:
Put a blank line before this line.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:63: a tuple of (info, URL, link,
tag, is_video, nickname)
"URL" --> "url"  (or else, in line 65 below, change "url:" into "URL:")

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:69: nickname: nickname of the video
for presentation
Put periods at the end of each sentence.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:74: # all videos are placed up to
the rows (below 15th row)
Capitalize the start of each sentence and put period at the end (throughout the
file)

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:75: is_video = (sub_type <=
self.__GetMaxValue(True))
I don't think the parens around the right-hand side of this assignment are
needed.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:76: file = cur_video + "/" +
cur_video + self.exts[sub_type]
When generating a path string, it's safer to use "os.path.join".

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:81: file, "'>", file,
'</a>&nbsp;&nbsp;', info, '</b>'])
Is there a reason you're creating the string this way, rather than using the
string formatting operator (here and below)?:

link = '<b><a href="%s">%s</a>&nbsp;&nbsp;%s</b>' % (url, file, info)

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:88: "playbackRate=1 loop valign=top
src='", file, "'>"])
Lines 83-88: the only difference between these two assignments is that one is
"video" and the other is "audio".  I recommend cutting down on the code like
this:

type = ['audio', 'video'][is_video]
tag = ''.join(["<", type, " id='v' controls autoplay ",
               "playbackRate=1 loop valign=top src='", file, "'>"])

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:97: Returns:
Put blank line before this line.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:101: return 15
The 15 should probably be defined as a named constant at the top of the file.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: return len(self.exts)-1
A shorter way to implement this function:

return [len(self.exts) - 1, 15][video_only]

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:107: """Generate random video info
can be used for playing this media.
"info can" --> "info that can"?

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:121: for i in range(10):
Maybe the value 10 should be an input to this function, so that it can change if
the user desires.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:125: if "not available" not in info:
Use "NOT_AVAILABLE_STRING".

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:155: (in compact form).
Summary line should fit on one line.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:175: tag = 'audio'
Lines 172-175:

tag = ['audio', 'video'][is_video]

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:185: compact_list: list generated by
bgenerateAllMediaInfosInCompactForm
"bgenerateAllMediaInfosInCompactForm" -->
"GenerateAllMediaInfosInCompactForm"

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:198: return l
This should only be indented 2 spaces underneath the line above it.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:25: 
Don't we need more unit tests?  For example, functions "GenerateRandomMediaInfo"
and "GenerateAllMediaInfos" do not seem to be called anywhere.

[imasaki1, 2011-03-17 23:29:18]

Updated based on CR comments. Thanks for following up.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1:
"exts","bbee","mobile","bear","sync","color","scout3d","buck","gromit","ironman","trek","dartmoor","still","tulip","chicken","3dtv","avatar","avatar3d","wembly","pitivi","qjoe","birds","red","crowd","parkrun","sonyh","garden"
On 2011/03/17 02:32:00, Nirnimesh wrote:
> Add some comments to explain the format of this csv.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:10: VIDEO_TEST_MATRIX_HOME =
'http://tskir-html5.kir.corp.google.com/testmatrix/'
On 2011/03/17 02:32:00, Nirnimesh wrote:
> Do not reference internal URLs in public scripts. Please keep these in
> pyauto_private in data dir.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:16: format.
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> The first line of this class docstring should be a 1-line summary, followed by
a
> blank line, followed by any  additional information.  I recommend you add some
> more details here about what this class does and how to use it.  What is a
> "video matrix file" that needs to be given as input?  What are the "different
> formats" that are output?

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:20: # video specifications or video
descriptions.
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Capitalize "v" in "video"

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:23: video_titles = []
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Are the above lists meant to be public attributes?  If so, you need to list
them
> in an "Attributes:" section within the class docstring above.  If not, I
believe
> it's convention to name them starting with an underscore.

I made them private instance variables.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:27: variables.
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> This should be a 1-line summary of this method.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:34:
http://tskir-html5.kir.corp.google.com/testmatrix/index.html
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> As Nirnimesh said, we should not reference internal URLs in public scripts.

Removed.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:37: audio files are stored more then
15th row
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Capitalize the start of each sentence and put periods at the end.  Also,
change
> "then" to "than" here.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:40: above
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Put period at the end of this sentence.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:44: for row in reader:
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> I think you can do:
> 
> for counter, row in enumerate(reader):
> 
> This will let you avoid having to initialize counter to 0 and having to
> increment it on each loop iteration below.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:53:
self.specs[self.video_titles[i]].append(row[i + 1])
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> It might be good to do some kind of error checking here.  This line assumes
that
> "self.video_titles[i]" is defined, which is true only if the CSV file is
> formatted according to your expectations.  If the CSV file is not formatted
> properly, I think an exception could be raised here.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:61: name: the name of the video file
(column name in the CSV file)
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Put periods at the end of each sentence in the above 2 lines.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:62: Returns:
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Put a blank line before this line.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:63: a tuple of (info, URL, link,
tag, is_video, nickname)
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> "URL" --> "url"  (or else, in line 65 below, change "url:" into "URL:")

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:69: nickname: nickname of the video
for presentation
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Put periods at the end of each sentence.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:74: # all videos are placed up to
the rows (below 15th row)
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Capitalize the start of each sentence and put period at the end (throughout
the
> file)

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:75: is_video = (sub_type <=
self.__GetMaxValue(True))
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> I don't think the parens around the right-hand side of this assignment are
> needed.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:76: file = cur_video + "/" +
cur_video + self.exts[sub_type]
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> When generating a path string, it's safer to use "os.path.join".

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:81: file, "'>", file,
'</a>&nbsp;&nbsp;', info, '</b>'])
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Is there a reason you're creating the string this way, rather than using the
> string formatting operator (here and below)?:
> 
> link = '<b><a href="%s">%s</a>&nbsp;&nbsp;%s</b>' % (url, file, info)

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:88: "playbackRate=1 loop valign=top
src='", file, "'>"])
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Lines 83-88: the only difference between these two assignments is that one is
> "video" and the other is "audio".  I recommend cutting down on the code like
> this:
> 
> type = ['audio', 'video'][is_video]
> tag = ''.join(["<", type, " id='v' controls autoplay ",
>                "playbackRate=1 loop valign=top src='", file, "'>"])

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:97: Returns:
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Put blank line before this line.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:101: return 15
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> The 15 should probably be defined as a named constant at the top of the file.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: return len(self.exts)-1
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> A shorter way to implement this function:
> 
> return [len(self.exts) - 1, 15][video_only]

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:107: """Generate random video info
can be used for playing this media.
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> "info can" --> "info that can"?

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:121: for i in range(10):
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Maybe the value 10 should be an input to this function, so that it can change
if
> the user desires.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:125: if "not available" not in info:
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Use "NOT_AVAILABLE_STRING".

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:155: (in compact form).
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Summary line should fit on one line.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:175: tag = 'audio'
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Lines 172-175:
> 
> tag = ['audio', 'video'][is_video]

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:185: compact_list: list generated by
bgenerateAllMediaInfosInCompactForm
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> "bgenerateAllMediaInfosInCompactForm" -->
> "GenerateAllMediaInfosInCompactForm"

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:186: target: taget nickname
On 2011/03/17 02:32:00, Nirnimesh wrote:
> need a blank line before Returns:
> 
> Repeat for all the methods above

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:196: [url, nickname, tag] = l
On 2011/03/17 02:32:00, Nirnimesh wrote:
> You can use:
> 
> for url, nickname, tag in compact_list:
> ...

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:197: if(target == nickname):
On 2011/03/17 02:32:00, Nirnimesh wrote:
> remove parens

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:198: return l
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> This should only be indented 2 spaces underneath the line above it.

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:17: data_file =
os.path.join('..', 'data', 'media', 'media_matrix_data.csv')
On 2011/03/17 02:32:00, Nirnimesh wrote:
> use self.DataDir()

Done.

http://codereview.chromium.org/6673078/diff/2001/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:25: 
On 2011/03/17 17:13:36, dennis_jeffrey wrote:
> Don't we need more unit tests?  For example, functions
"GenerateRandomMediaInfo"
> and "GenerateAllMediaInfos" do not seem to be called anywhere.

Done.

[dennis_jeffrey, 2011-03-18 17:47:49]

Thank you for addressing my earlier comments.  I think this looks a lot better
now!  I'm including some more comments below.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1: "The file contains matrix (video
type (columns) and its codec (rows)) and each entry has its specification (or
"not available"),"video codec are up to row 15 and rests are audio codec",
Rather than forcing video codecs to exist up to row 15 and having the rest be
audio codecs, is it possible to simply prefix each row with either "video" or
"audio" to indicate which type it is?  That way the CSV file is not restricted
to a particular ordering or number of rows.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:43: Audio files are stored more then
15th row.
"more then" --> "more than"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:44: Args:
Put a blank line right before the "Args:" line.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:46: above.
Since this function raises csv.Error in line 68 below, we need a "Raises:"
section in this function docstring.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:58: if counter == 1:
"if" --> "elif"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:66: print "Row %d should have %d
columns but have %d columns" % (counter,
"but have" --> "but has"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:90: tag: a tag string can be used to
display video or audio.
Put the line for "tag" above the line for "is_video", so it matches the same
order as in the tuple above at line 85.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:93: if name is "none":
Use single quotes instead of double quotes here.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:96: # All videos are placed up to
the rows (below 15th row).
# All videos are placed up to row MAX_VIDEO_INDEX.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: tag_attr = "id='v' controls
autoplay playbackRate=1 loop valign=top"
Please use single quotes to surround the outer string:

Either:

tag_attr = 'id=\'v\' controls autoplay playbackRate=1 loop valign=top'

Or:

tag_attr = 'id="v" controls autoplay playbackRate=1 loop valign=top'

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:105: tag = "<%s %s src='%s'>" %
(type, tag_attr, file)
Same comment as line 104 above.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:138: # Try number_of_tries times to
find avaialble video/audio.
"avaialble" --> "available"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:145: video_matrix_home_url)
I think lines 143-145 might look better like this:

info, url, link, tag, is_video, nickname = (
    self.__GenerateMediaInfo(sub_type, name, video_matrix_home_url))

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:147: return  (info, url, link, tag,
is_video, nickname)
Remove one of the extra spaces after the word "return"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:148: # Gives up after that (very
small chance)
Put period at end of sentence.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:161: is_video: True if the file is
video; False otherwise.
What about the "link" and "tag" tuple items?

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:172: media_matrix_home_url)
Similar comment as line 145 above.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:200: media_matrix_home_url)
Similar comment as line 145 above.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:205: 
Functions GenerateAllMediaInfosInCompactForm, GenerateAllMediaInfos, and
GenerateRandomMediaInfo all have some similar code within each function.  You
don't have to do this now, but you might want to consider seeing if you can
write a single, more general function that takes care of the work of each of
these individual functions.  It may lessen the total amount of code and make it
easier to maintain.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:215: A tuple (info, url, link, tag,
is_video, nickname) where nickname.
I don't understand what you mean by "where nickname" here.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:219: (such as bear.webm).
What about the "info", "link", and "is_video" tuple items?

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:16: def
__readCSVFile(self):
In the function name, capitalize the "r" at the beginning.  Function names
should start with a capital letter when possible (one exception is for the
"test" functions below, which need to start with a lower-case as per the
unittest module).

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:17: """read CSV file for
test and returns a media test matrix object.
Capitalize the "r" in "read" and change it to "Reads"

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:20: a media test matrix
Capitalize the starting "a", and add a period at the end.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:33: media_test_matrix_list,
"mobile0.webm")
When the function call in the previous line doesn't include any parameters, the
next line should be indented 4 spaces rather than 2.  So this line should be
indented by 2 additional spaces.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34:
self.assertEqual(mobile_test_info[1], "mobile0.webm")
Prefer single quotes to double quotes in strings, here and at line 33 above.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34:
self.assertEqual(mobile_test_info[1], "mobile0.webm")
Before accessing position 1 of the mobile_test_info list, you might want to
verify that the list does have at least one element.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:41: is_video, nickname) =
matrix.GenerateRandomMediaInfo()
I think lines 40-41 might look better like this:

info, url, link, tag, is_video, nickname = (
    matrix.GenerateRandomMediaInfo())

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:42: self.assertTrue(info is
not None)
Thanks for adding these extra tests.  Could you also check to make sure there
are no more functions that are missing unit tests?

[imasaki1, 2011-03-18 21:54:25]

Thanks for looking this this. Here is modification based on your CR CR comments.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1: "The file contains matrix (video
type (columns) and its codec (rows)) and each entry has its specification (or
"not available"),"video codec are up to row 15 and rests are audio codec",
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Rather than forcing video codecs to exist up to row 15 and having the rest be
> audio codecs, is it possible to simply prefix each row with either "video" or
> "audio" to indicate which type it is?  That way the CSV file is not restricted
> to a particular ordering or number of rows.

Added this in the second column

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:43: Audio files are stored more then
15th row.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> "more then" --> "more than"

Removed since we use video/audio tag.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:44: Args:
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Put a blank line right before the "Args:" line.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:46: above.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Since this function raises csv.Error in line 68 below, we need a "Raises:"
> section in this function docstring.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:58: if counter == 1:
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> "if" --> "elif"

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:66: print "Row %d should have %d
columns but have %d columns" % (counter,
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> "but have" --> "but has"

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:90: tag: a tag string can be used to
display video or audio.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Put the line for "tag" above the line for "is_video", so it matches the same
> order as in the tuple above at line 85.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:93: if name is "none":
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Use single quotes instead of double quotes here.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:96: # All videos are placed up to
the rows (below 15th row).
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> # All videos are placed up to row MAX_VIDEO_INDEX.

Removed since we are not using MAX_VIDEO_INDEX anymore

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: tag_attr = "id='v' controls
autoplay playbackRate=1 loop valign=top"
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Please use single quotes to surround the outer string:
> 
> Either:
> 
> tag_attr = 'id=\'v\' controls autoplay playbackRate=1 loop valign=top'
> 
> Or:
> 
> tag_attr = 'id="v" controls autoplay playbackRate=1 loop valign=top'

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:105: tag = "<%s %s src='%s'>" %
(type, tag_attr, file)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Same comment as line 104 above.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:138: # Try number_of_tries times to
find avaialble video/audio.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> "avaialble" --> "available"

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:145: video_matrix_home_url)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> I think lines 143-145 might look better like this:
> 
> info, url, link, tag, is_video, nickname = (
>     self.__GenerateMediaInfo(sub_type, name, video_matrix_home_url))

Changed as well as other places.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:147: return  (info, url, link, tag,
is_video, nickname)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Remove one of the extra spaces after the word "return"

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:148: # Gives up after that (very
small chance)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Put period at end of sentence.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:161: is_video: True if the file is
video; False otherwise.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> What about the "link" and "tag" tuple items?

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:172: media_matrix_home_url)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Similar comment as line 145 above.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:200: media_matrix_home_url)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Similar comment as line 145 above.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:205: 
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Functions GenerateAllMediaInfosInCompactForm, GenerateAllMediaInfos, and
> GenerateRandomMediaInfo all have some similar code within each function.  You
> don't have to do this now, but you might want to consider seeing if you can
> write a single, more general function that takes care of the work of each of
> these individual functions.  It may lessen the total amount of code and make
it
> easier to maintain.

I agree. I will add TODO

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:215: A tuple (info, url, link, tag,
is_video, nickname) where nickname.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> I don't understand what you mean by "where nickname" here.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:219: (such as bear.webm).
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> What about the "info", "link", and "is_video" tuple items?

It is using compact form where we do not need these.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:16: def
__readCSVFile(self):
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> In the function name, capitalize the "r" at the beginning.  Function names
> should start with a capital letter when possible (one exception is for the
> "test" functions below, which need to start with a lower-case as per the
> unittest module).

Done also make this private

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:17: """read CSV file for
test and returns a media test matrix object.
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Capitalize the "r" in "read" and change it to "Reads"

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:20: a media test matrix
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Capitalize the starting "a", and add a period at the end.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:33: media_test_matrix_list,
"mobile0.webm")
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> When the function call in the previous line doesn't include any parameters,
the
> next line should be indented 4 spaces rather than 2.  So this line should be
> indented by 2 additional spaces.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34:
self.assertEqual(mobile_test_info[1], "mobile0.webm")
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Prefer single quotes to double quotes in strings, here and at line 33 above.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34:
self.assertEqual(mobile_test_info[1], "mobile0.webm")
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Before accessing position 1 of the mobile_test_info list, you might want to
> verify that the list does have at least one element.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34:
self.assertEqual(mobile_test_info[1], "mobile0.webm")
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Prefer single quotes to double quotes in strings, here and at line 33 above.

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:41: is_video, nickname) =
matrix.GenerateRandomMediaInfo()
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> I think lines 40-41 might look better like this:
> 
> info, url, link, tag, is_video, nickname = (
>     matrix.GenerateRandomMediaInfo())

Done.

http://codereview.chromium.org/6673078/diff/7002/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:42: self.assertTrue(info is
not None)
On 2011/03/18 17:47:50, dennis_jeffrey wrote:
> Thanks for adding these extra tests.  Could you also check to make sure there
> are no more functions that are missing unit tests?

Done.

[dennis_jeffrey, 2011-03-18 23:51:41]

It's looking good!  Just a few more comments.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1: "The file contains matrix (video
type (columns) and its codec (rows)) and each entry has its specification (or
"not available"),"video codec are up to row 15 and rests are audio codec",
We should modify this comment now that it's no longer required for video codecs
to be up to row 15 :-)

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: # All videos are placed up to
the rows (below 15th row).
I think we can remove this comment now that videos aren't restricted to being in
the first 15 rows.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:105: is_video =
self.__tags[self.__exts[sub_type]] == "video"
Use single quotes for strings:

'video'

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:150: video_only: True if generate
random video only (not audio).
Add a line here for the "media_matrix_home_url" arg.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:161: video_infos = []
Since it looks like this function processes both video and audio (depending on
the "video_only" flag), maybe we should change the name of this variable to
something more general like "media_infos".

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:180: video_only: True if generate
random video only.
Need a line for the "media_matrix_home_url" arg.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:189: video_infos = []
Similar comment as line 161 above.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:210: A tuple (info, url, link, tag,
is_video, nickname) where nickname is
Since this function only returns (url, nickname, tag), we need to change the
comment here to reflect that.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:214: nickname: nickname of the
video/audio for presentation
Swap the "nickname" and "tag" lines to match the order in which items are
specified in the tuple in line 219 below.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:32:
self.assertEqual(len(media_test_matrix_list), 249)
In all assert function calls, I believe there's an optional parameter called
"msg" which you can specify to give an informative error message in the event
that the assertion fails.  You may want to consider adding that to all asserts
in this file, so that more informative error messages can be printed when there
are failures.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34: # Get all media data.
Move this comment right before line 33 above.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:51: self.assertTrue(info is
not None)
Should we assert on any of the other return values?  (url, link, tag, is_video,
nickname)?

[imasaki1, 2011-03-19 01:00:38]

Thank you for quick turn around.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/data/media/media...
File chrome/test/data/media/media_matrix_data.csv (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/data/media/media...
chrome/test/data/media/media_matrix_data.csv:1: "The file contains matrix (video
type (columns) and its codec (rows)) and each entry has its specification (or
"not available"),"video codec are up to row 15 and rests are audio codec",
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> We should modify this comment now that it's no longer required for video
codecs
> to be up to row 15 :-)

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:104: # All videos are placed up to
the rows (below 15th row).
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> I think we can remove this comment now that videos aren't restricted to being
in
> the first 15 rows.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:105: is_video =
self.__tags[self.__exts[sub_type]] == "video"
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Use single quotes for strings:
> 
> 'video'

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:150: video_only: True if generate
random video only (not audio).
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Add a line here for the "media_matrix_home_url" arg.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:161: video_infos = []
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Since it looks like this function processes both video and audio (depending on
> the "video_only" flag), maybe we should change the name of this variable to
> something more general like "media_infos".

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:180: video_only: True if generate
random video only.
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Need a line for the "media_matrix_home_url" arg.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:189: video_infos = []
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Similar comment as line 161 above.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:210: A tuple (info, url, link, tag,
is_video, nickname) where nickname is
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Since this function only returns (url, nickname, tag), we need to change the
> comment here to reflect that.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:214: nickname: nickname of the
video/audio for presentation
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Swap the "nickname" and "tag" lines to match the order in which items are
> specified in the tuple in line 219 below.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:32:
self.assertEqual(len(media_test_matrix_list), 249)
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> In all assert function calls, I believe there's an optional parameter called
> "msg" which you can specify to give an informative error message in the event
> that the assertion fails.  You may want to consider adding that to all asserts
> in this file, so that more informative error messages can be printed when
there
> are failures.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:34: # Get all media data.
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Move this comment right before line 33 above.

Done.

http://codereview.chromium.org/6673078/diff/7003/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:51: self.assertTrue(info is
not None)
On 2011/03/18 23:51:41, dennis_jeffrey wrote:
> Should we assert on any of the other return values?  (url, link, tag,
is_video,
> nickname)?

Done.

[Nirnimesh, 2011-03-20 09:14:06]

Thanks guys for the quick turnaround on this.

No need to wait for additional review from me on this, after addressing the
comments I made, and after LGTM from Dennis.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:28: __exts = []
nit: why double underscores?
(Repeat for all occurrences below)

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:83: def __GenerateMediaInfo(self,
sub_type, name, media_test_matrix_home_url):
nit: why double underscores?

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:9: import unittest
nit: Please leave a blank line after all the system imports and below your local
imports

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:16: def
__ReadCSVFile(self):
nit: use single underscore

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:24: data_file =
os.path.join('..', 'data', 'media', 'media_matrix_data.csv')
use os.pardir instead of '..'

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:52: "total number of
elements in the returned list is wrong")
nit: use single quote char ' please
Repeat elsewhere in this script

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:65: 
leave another blank line here.
Need 2 blank lines at global scope

[imasaki1, 2011-03-20 16:17:15]

Thank you for cr this on weekends. I really appreciate this.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:28: __exts = []
On 2011/03/20 09:14:06, Nirnimesh wrote:
> nit: why double underscores?
> (Repeat for all occurrences below)

Changed them into single underscore.

(I thought these are for private methods/instance variables)

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix.py:83: def __GenerateMediaInfo(self,
sub_type, name, media_test_matrix_home_url):
On 2011/03/20 09:14:06, Nirnimesh wrote:
> nit: why double underscores?

I thought it is for private methods when I read some tutorials.... Changed them
into single understores.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
File chrome/test/functional/media_test_matrix_unittest.py (right):

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:9: import unittest
On 2011/03/20 09:14:06, Nirnimesh wrote:
> nit: Please leave a blank line after all the system imports and below your
local
> imports

Done.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:16: def
__ReadCSVFile(self):
On 2011/03/20 09:14:06, Nirnimesh wrote:
> nit: use single underscore

Done.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:24: data_file =
os.path.join('..', 'data', 'media', 'media_matrix_data.csv')
On 2011/03/20 09:14:06, Nirnimesh wrote:
> use os.pardir instead of '..'

Done.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:52: "total number of
elements in the returned list is wrong")
On 2011/03/20 09:14:06, Nirnimesh wrote:
> nit: use single quote char ' please
> Repeat elsewhere in this script

Done.

http://codereview.chromium.org/6673078/diff/7004/chrome/test/functional/media...
chrome/test/functional/media_test_matrix_unittest.py:65: 
On 2011/03/20 09:14:06, Nirnimesh wrote:
> leave another blank line here.
> Need 2 blank lines at global scope

Done.

[dennis_jeffrey, 2011-03-22 00:10:00]

LGTM

Just a few minor comments below, but looks good!  Thanks for addressing all my
comments.

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:150: media_test_matrix_home_url: url
of the matrix home that used
"that used" --> "that is used"

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:150: media_test_matrix_home_url: url
of the matrix home that used
"media_test_matrix_home_url"
-->
"media_matrix_home_url"

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:181: media_test_matrix_home_url: url
of the matrix home that used
"media_test_matrix_home_url"
-->
"media_matrix_home_url"

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:181: media_test_matrix_home_url: url
of the matrix home that used
"that used" --> "that is used"

[imasaki1, 2011-03-22 00:22:12]

Modified based on final comments. Do you think you can commit this for me since
I do not have commit access?

Thank you for CR comments.

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
File chrome/test/functional/media_test_matrix.py (right):

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:150: media_test_matrix_home_url: url
of the matrix home that used
On 2011/03/22 00:10:00, dennis_jeffrey wrote:
> "that used" --> "that is used"

Done.

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:150: media_test_matrix_home_url: url
of the matrix home that used
On 2011/03/22 00:10:00, dennis_jeffrey wrote:
> "that used" --> "that is used"

Done.

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:181: media_test_matrix_home_url: url
of the matrix home that used
On 2011/03/22 00:10:00, dennis_jeffrey wrote:
> "media_test_matrix_home_url"
> -->
> "media_matrix_home_url"

Done.

http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
chrome/test/functional/media_test_matrix.py:181: media_test_matrix_home_url: url
of the matrix home that used
On 2011/03/22 00:10:00, dennis_jeffrey wrote:
> "that used" --> "that is used"

Done.

[dennisjeffrey, 2011-03-22 00:30:37]

Good question - I just recently became a provisional committer and committed
my first CL as one this morning.  Does anybody CC'ed know if it's ok for me
to commit Kenji's CL?

-Dennis

On Mon, Mar 21, 2011 at 5:22 PM, <imasaki@chromium.org> wrote:

> Modified based on final comments. Do you think you can commit this for me
> since
> I do not have commit access?
>
> Thank you for CR comments.
>
>
>
>
>
>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
> File chrome/test/functional/media_test_matrix.py (right):
>
>
>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
> chrome/test/functional/media_test_matrix.py:150:
> media_test_matrix_home_url: url of the matrix home that used
> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>
>> "that used" --> "that is used"
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
> chrome/test/functional/media_test_matrix.py:150:
> media_test_matrix_home_url: url of the matrix home that used
> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>
>> "that used" --> "that is used"
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
> chrome/test/functional/media_test_matrix.py:181:
> media_test_matrix_home_url: url of the matrix home that used
> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>
>> "media_test_matrix_home_url"
>> -->
>> "media_matrix_home_url"
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
> chrome/test/functional/media_test_matrix.py:181:
> media_test_matrix_home_url: url of the matrix home that used
> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>
>> "that used" --> "that is used"
>>
>
> Done.
>
>
> http://codereview.chromium.org/6673078/
>

[dennis_jeffrey, 2011-03-22 00:45:49]

Somebody told me once that it's sometimes better to ask for forgiveness than
for permission.  I'm going to go ahead and try committing this CL for Kenji.

-Dennis


On Mon, Mar 21, 2011 at 5:30 PM, Dennis Jeffrey <dennisjeffrey@google.com>wrote:

> Good question - I just recently became a provisional committer and
> committed my first CL as one this morning.  Does anybody CC'ed know if it's
> ok for me to commit Kenji's CL?
>
> -Dennis
>
>
> On Mon, Mar 21, 2011 at 5:22 PM, <imasaki@chromium.org> wrote:
>
>> Modified based on final comments. Do you think you can commit this for me
>> since
>> I do not have commit access?
>>
>> Thank you for CR comments.
>>
>>
>>
>>
>>
>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>> File chrome/test/functional/media_test_matrix.py (right):
>>
>>
>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>> chrome/test/functional/media_test_matrix.py:150:
>> media_test_matrix_home_url: url of the matrix home that used
>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>
>>> "that used" --> "that is used"
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>> chrome/test/functional/media_test_matrix.py:150:
>> media_test_matrix_home_url: url of the matrix home that used
>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>
>>> "that used" --> "that is used"
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>> chrome/test/functional/media_test_matrix.py:181:
>> media_test_matrix_home_url: url of the matrix home that used
>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>
>>> "media_test_matrix_home_url"
>>> -->
>>> "media_matrix_home_url"
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>> chrome/test/functional/media_test_matrix.py:181:
>> media_test_matrix_home_url: url of the matrix home that used
>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>
>>> "that used" --> "that is used"
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/6673078/
>>
>
>

[dennisjeffrey, 2011-03-22 00:52:19]

Committed as r78952.

On Mon, Mar 21, 2011 at 5:45 PM, Dennis Jeffrey
<dennisjeffrey@chromium.org>wrote:

> Somebody told me once that it's sometimes better to ask for forgiveness
> than for permission.  I'm going to go ahead and try committing this CL for
> Kenji.
>
> -Dennis
>
>
> On Mon, Mar 21, 2011 at 5:30 PM, Dennis Jeffrey
<dennisjeffrey@google.com>wrote:
>
>> Good question - I just recently became a provisional committer and
>> committed my first CL as one this morning.  Does anybody CC'ed know if it's
>> ok for me to commit Kenji's CL?
>>
>> -Dennis
>>
>>
>> On Mon, Mar 21, 2011 at 5:22 PM, <imasaki@chromium.org> wrote:
>>
>>> Modified based on final comments. Do you think you can commit this for me
>>> since
>>> I do not have commit access?
>>>
>>> Thank you for CR comments.
>>>
>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>> File chrome/test/functional/media_test_matrix.py (right):
>>>
>>>
>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>> chrome/test/functional/media_test_matrix.py:150:
>>> media_test_matrix_home_url: url of the matrix home that used
>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>
>>>> "that used" --> "that is used"
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>> chrome/test/functional/media_test_matrix.py:150:
>>> media_test_matrix_home_url: url of the matrix home that used
>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>
>>>> "that used" --> "that is used"
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>> chrome/test/functional/media_test_matrix.py:181:
>>> media_test_matrix_home_url: url of the matrix home that used
>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>
>>>> "media_test_matrix_home_url"
>>>> -->
>>>> "media_matrix_home_url"
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>> chrome/test/functional/media_test_matrix.py:181:
>>> media_test_matrix_home_url: url of the matrix home that used
>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>
>>>> "that used" --> "that is used"
>>>>
>>>
>>> Done.
>>>
>>>
>>> http://codereview.chromium.org/6673078/
>>>
>>
>>
>

[Nirnimesh, 2011-03-22 13:49:16]

Dennis, it's perfectly fine for you to commit Kenji's CL. Thanks for doing
this.

On Tue, Mar 22, 2011 at 6:22 AM, Dennis Jeffrey <dennisjeffrey@google.com>wrote:

> Committed as r78952.
>
> On Mon, Mar 21, 2011 at 5:45 PM, Dennis Jeffrey <
> dennisjeffrey@chromium.org> wrote:
>
>> Somebody told me once that it's sometimes better to ask for forgiveness
>> than for permission.  I'm going to go ahead and try committing this CL for
>> Kenji.
>>
>> -Dennis
>>
>>
>> On Mon, Mar 21, 2011 at 5:30 PM, Dennis Jeffrey <dennisjeffrey@google.com
>> > wrote:
>>
>>> Good question - I just recently became a provisional committer and
>>> committed my first CL as one this morning.  Does anybody CC'ed know if it's
>>> ok for me to commit Kenji's CL?
>>>
>>> -Dennis
>>>
>>>
>>> On Mon, Mar 21, 2011 at 5:22 PM, <imasaki@chromium.org> wrote:
>>>
>>>> Modified based on final comments. Do you think you can commit this for
>>>> me since
>>>> I do not have commit access?
>>>>
>>>> Thank you for CR comments.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>>> File chrome/test/functional/media_test_matrix.py (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>>> chrome/test/functional/media_test_matrix.py:150:
>>>> media_test_matrix_home_url: url of the matrix home that used
>>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>>
>>>>> "that used" --> "that is used"
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>>> chrome/test/functional/media_test_matrix.py:150:
>>>> media_test_matrix_home_url: url of the matrix home that used
>>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>>
>>>>> "that used" --> "that is used"
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>>> chrome/test/functional/media_test_matrix.py:181:
>>>> media_test_matrix_home_url: url of the matrix home that used
>>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>>
>>>>> "media_test_matrix_home_url"
>>>>> -->
>>>>> "media_matrix_home_url"
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6673078/diff/11002/chrome/test/functional/medi...
>>>> chrome/test/functional/media_test_matrix.py:181:
>>>> media_test_matrix_home_url: url of the matrix home that used
>>>> On 2011/03/22 00:10:00, dennis_jeffrey wrote:
>>>>
>>>>> "that used" --> "that is used"
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>> http://codereview.chromium.org/6673078/
>>>>
>>>
>>>
>>
>


-- 
../NiR