Bug 62226 – nrwt: support webaudio in chromium driver

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: support webaudio in chromium driver

[Dirk Pranke]

Created attachment 96281 [details]
Patch

[Dirk Pranke]

Created attachment 96282 [details]
fix uninitialized variables

[Dirk Pranke]

Created attachment 96328 [details]
fix bugs uncovered in testing (base64 import, encoding header)

[Tony Chang]

Comment on attachment 96328 [details]
fix bugs uncovered in testing (base64 import, encoding header)

View in context: https://bugs.webkit.org/attachment.cgi?id=96328&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:530
> +                if has_audio:
> +                    audio.append(line)
> +                else:
> +                    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.

[Chris Rogers]

We're planning on either having text output or audio output, but not both in the same test.

(In reply to comment #4)
> (From update of attachment 96328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96328&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:530
> > +                if has_audio:
> > +                    audio.append(line)
> > +                else:
> > +                    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.

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 96328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96328&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:530
> > +                if has_audio:
> > +                    audio.append(line)
> > +                else:
> > +                    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?

[Tony Chang]

Comment on attachment 96328 [details]
fix bugs uncovered in testing (base64 import, encoding header)

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))

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 96328 [details])
> 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))

OK.

[Dirk Pranke]

Created attachment 96515 [details]
update w/ review feedback

[Tony Chang]

Comment on attachment 96515 [details]
update w/ review feedback

Should we have a test case for this?

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 96515 [details])
> Should we have a test case for this?

Yeah, that would be good, but we don't quite have the right infrastructure to run the tests cleanly. I need to update the mock_drt code and figure out how to wire it in for this sort of testing. Shouldn't be too hard and I should be able to get to it soon.

[Dirk Pranke]

Comment on attachment 96515 [details]
update w/ review feedback

Clearing flags on attachment: 96515

Committed r88483: <http://trac.webkit.org/changeset/88483>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.