Enable web audio IPP for x86 chromium android
Created attachment 153757 [details] Patch
use static link for android.
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?
Created attachment 153761 [details] Patch
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
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.
(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!
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 [].
Created attachment 153975 [details] Patch
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
Created attachment 155195 [details] Patch
Comment on attachment 155195 [details] Patch re-submit the patch for win-ews issue.
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
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
Created attachment 155387 [details] Patch
(In reply to comment #15) > Created an attachment (id=155387) [details] > Patch The patch is identical to the previous one, as such, LGTM. Thanks :).
(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?
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.
(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.
Created attachment 155390 [details] Patch
(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.
(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.
(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!
Comment on attachment 155390 [details] Patch Clearing flags on attachment: 155390 Committed r124154: <http://trac.webkit.org/changeset/124154>
All reviewed patches have been landed. Closing bug.