[webkit-reviews] review denied: [Bug 62226] nrwt: support webaudio in chromium driver : [Attachment 96328] fix bugs uncovered in testing (base64 import, encoding header)

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 62226: nrwt: support webaudio in chromium driver
https://bugs.webkit.org/show_bug.cgi?id=62226

Attachment 96328: fix bugs uncovered in testing (base64 import, encoding
header)
https://bugs.webkit.org/attachment.cgi?id=96328&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96328&action=review

>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:530
>>>> +			  output.append(line)
>>> 
>>> Is it possible to have audio and other output from the same test?  If so, I
guess the text output has to come first and there's an audio header in the
middle?  If not, seems like we don't need a separate |audio| variable.
>> 
>> 
> 
> As Chris noted, we can't have both audio and text output. So, we could reuse
the output variable, but we'd have to have a branch later in the function to
make sure that the output is passed to the audio= param of the DriverOutput
constructor instead of the text= param.
> 
> I thought having a single constructor call in the end was better, but it
might help to rename output to 'text' or something like that to make it
clear(er) that that variable was only ever used for a single type of output as
well. 
> 
> WDYT?

I think using audio_bytes is fine.  You can still do that.  Something like:

  audio_bytes = None
  if has_audio:
      if has_base64:
	  audio_bytes = base64.b64decode(''.join(output))
      else:
	  audio_bytes = ''.join(output)
  else:
      text = ''.join(output)
      if not text:
	  text = None

  return base.DriverOutput(text, output_image, actual_checksum,
audio=audio_bytes,
      crash=crash, test_time=run_time, timeout=timeout, error=''.join(error))