Bug 91973 – [Chromium] Enable web audio IPP for x86 chromium android

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

Enable web audio IPP for x86 chromium android

[Wei James (wistoch)]

Created attachment 153757 [details]
Patch

[Wei James (wistoch)]

use static link for android.

[Peter Beverloo]

Comment on attachment 153757 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Enable web audio IPP for x86 chromium android

It'd be great if you could prefix this with [Chromium], since it only touches Chromium-specific code. A brief explanation in the ChangeLog about what this change does would be useful too, despite it being a minor change. Something among the lines of "Include the IPP libraries at link-time for Android builds when compiling the Web Audio API with IPP support." would work.

> Source/WebCore/WebCore.gyp/WebCore.gyp:2123
> +        # Use static link for Android.

Maybe emphasize that the libraries are x86-libraries, different from the often assumed ARM?

[Wei James (wistoch)]

Created attachment 153761 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 153757 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Enable web audio IPP for x86 chromium android
> 
> It'd be great if you could prefix this with [Chromium], since it only touches Chromium-specific code. A brief explanation in the ChangeLog about what this change does would be useful too, despite it being a minor change. Something among the lines of "Include the IPP libraries at link-time for Android builds when compiling the Web Audio API with IPP support." would work.

fixed. thanks

>> Source/WebCore/WebCore.gyp/WebCore.gyp:2123
>> +        # Use static link for Android.
> 
> Maybe emphasize that the libraries are x86-libraries, different from the often assumed ARM?

fixed. thanks

[Peter Beverloo]

LGTM from Android's side, thank you! I also confirmed that the normal (non-x86) build works correctly.

Please wait for a formal review before submitting this. It's probably fine to ask anyone to rubberstamp this as it's a really isolated change.

[Wei James (wistoch)]

(In reply to comment #6)
> LGTM from Android's side, thank you! I also confirmed that the normal (non-x86) build works correctly.
> 
> Please wait for a formal review before submitting this. It's probably fine to ask anyone to rubberstamp this as it's a really isolated change.

thanks!

[Tony Chang]

Comment on attachment 153761 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:1234
> +        ['OS in ["linux", "android"] and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {

Nit: Use a tuple () instead of a list [].

> Source/WebCore/WebCore.gyp/WebCore.gyp:1437
> +        ['OS in ["linux", "android"] and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {

Nit: Use a tuple () instead of a list [].

[Wei James (wistoch)]

Created attachment 153975 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 153761 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1234
>> +        ['OS in ["linux", "android"] and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> 
> Nit: Use a tuple () instead of a list [].

fixed. thanks

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1437
>> +        ['OS in ["linux", "android"] and "WTF_USE_WEBAUDIO_IPP=1" in feature_defines', {
> 
> Nit: Use a tuple () instead of a list [].

fixed. thanks

[Wei James (wistoch)]

Created attachment 155195 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 155195 [details]
Patch

re-submit the patch for win-ews issue.

[WebKit Review Bot]

Comment on attachment 155195 [details]
Patch

Attachment 155195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13394182

New failing tests:
platform/chromium/compositing/accelerated-drawing/svg-filters.html
platform/chromium/compositing/accelerated-drawing/alpha.html

[WebKit Review Bot]

Created attachment 155197 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Wei James (wistoch)]

Created attachment 155387 [details]
Patch

[Peter Beverloo]

(In reply to comment #15)
> Created an attachment (id=155387) [details]
> Patch

The patch is identical to the previous one, as such, LGTM. Thanks :).

[Wei James (wistoch)]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=155387) [details] [details]
> > Patch
> 
> The patch is identical to the previous one, as such, LGTM. Thanks :).

yes. the cr-linux bot failed, but I think it is not caused by this patch. 

so I re-submit it to try it again. 

how can I trigger the trybot without re-submitting the patch?

[Peter Beverloo]

Also, since Tony already r+'ed your patch and you made no code changes except for his review feedback, it's fine to change the:

"Reviewed by NOBODY (OOPS!)."

line in the review to read:

"Reviewed by Tony Chang."

Then, when uploading your patch, use the "webkit-patch upload --no-review --request-commit" command. This will not set "r?" (because it has already been reviewed!), but instead sets "cq?", which is short for requesting commit queue. Any committer can set that (including you, when you get your commit rights), which often makes it easier to land the patch. For other reviewers it makes it clear that the patch has already been reviewed.

[Peter Beverloo]

(In reply to comment #17)
> yes. the cr-linux bot failed, but I think it is not caused by this patch. 
> 
> so I re-submit it to try it again. 
> 
> how can I trigger the trybot without re-submitting the patch?

You can't.. The failure seen by the bot was unrelated.

[Wei James (wistoch)]

Created attachment 155390 [details]
Patch

[Tony Chang]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Created an attachment (id=155387) [details] [details] [details]
> > > Patch
> > 
> > The patch is identical to the previous one, as such, LGTM. Thanks :).
> 
> yes. the cr-linux bot failed, but I think it is not caused by this patch. 
> 
> so I re-submit it to try it again. 
> 
> how can I trigger the trybot without re-submitting the patch?

You can just cq? again and anyone can cq+ the same patch to try again.

[Wei James (wistoch)]

(In reply to comment #18)
> Also, since Tony already r+'ed your patch and you made no code changes except for his review feedback, it's fine to change the:
> 
> "Reviewed by NOBODY (OOPS!)."
> 
> line in the review to read:
> 
> "Reviewed by Tony Chang."
> 
> Then, when uploading your patch, use the "webkit-patch upload --no-review --request-commit" command. This will not set "r?" (because it has already been reviewed!), but instead sets "cq?", which is short for requesting commit queue. Any committer can set that (including you, when you get your commit rights), which often makes it easier to land the patch. For other reviewers it makes it clear that the patch has already been reviewed.

peter, thanks a lot for the instruction.

[Wei James (wistoch)]

(In reply to comment #21)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Created an attachment (id=155387) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > The patch is identical to the previous one, as such, LGTM. Thanks :).
> > 
> > yes. the cr-linux bot failed, but I think it is not caused by this patch. 
> > 
> > so I re-submit it to try it again. 
> > 
> > how can I trigger the trybot without re-submitting the patch?
> 
> You can just cq? again and anyone can cq+ the same patch to try again.

tony, thanks!

so committer can cq+ without waiting for all bots green? I have thought you will wait for all bots green and then r+ and cq+. 

thanks!

[WebKit Review Bot]

Comment on attachment 155390 [details]
Patch

Clearing flags on attachment: 155390

Committed r124154: <http://trac.webkit.org/changeset/124154>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.