Bug 98733

Created attachment 167718 [details] Patch

Comment on attachment 167718 [details] Patch Attachment 167718 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14218639

Created attachment 167722 [details] Patch

Comment on attachment 167722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167722&action=review > Source/WebCore/GNUmakefile.list.am:4362 > + Source/WebCore/platform/graphics/filters/StillCustomFilterProgram.h \ How about we call it WebCustomFilterProgram, and put it in WebKit2/Shared? > Source/WebCore/rendering/style/StyleCustomFilterProgram.h:60 > + virtual bool isStyleCustomFilterProgram() const OVERRIDE { return true; } > + Do you over have a StillCustomFilterProgram and a StyleCustomFilterProgram in the same process? I don't think these comparisons are necessary.

(In reply to comment #4) > How about we call it WebCustomFilterProgram, and put it in WebKit2/Shared? Thanks for suggestions. I actually did not know where is the best place. :) > Do you over have a StillCustomFilterProgram and a StyleCustomFilterProgram in the same process? > I don't think these comparisons are necessary. No, always lay in separate processes. Absolutely!

Created attachment 167854 [details] Patch

Comment on attachment 167854 [details] Patch I don't think we want to allow sending arbitrary shader strings from the web process to the UI process.

(In reply to comment #7) > (From update of attachment 167854 [details]) > I don't think we want to allow sending arbitrary shader strings from the web process to the UI process. We want to allow this for coordinated graphics (Qt/EFL). Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)?

(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 167854 [details] [details]) > > I don't think we want to allow sending arbitrary shader strings from the web process to the UI process. > > We want to allow this for coordinated graphics (Qt/EFL). > Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)? Though in any case we would want to validate the shader first, and then serialize the CustomFilterValidatedProgram.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (From update of attachment 167854 [details] [details] [details]) > > > I don't think we want to allow sending arbitrary shader strings from the web process to the UI process. > > > > We want to allow this for coordinated graphics (Qt/EFL). > > Any opposition to doing this if it's guarded with USE(COORDINATED_GRAPHICS)? CSS_FILTERS and CSS_SHADERS guard are already in the USE(COORDINATED_GRAPHICS). > > Though in any case we would want to validate the shader first, and then serialize the CustomFilterValidatedProgram. Thank Anders and Noam for very valid concerns. The most critical requirement is that UI TexMap never execute author's arbitrary shaders. I think there are two solutions: 1. As Noam mentioned, validate the shader first, and then serialize the CustomFilterValidatedProgram. 2. Serialize CustomFilterProgram, and validate the shader using CustomFilterValidatedProgram. I prefer the second solution, because : 1. Texmap will receive CustomFilterOperation that has CustomFilterProgram as a member variable. 2. We can validate arbitrary shaders using ANGLE validator with the SH_CSS_SHADERS_SPEC flag like CustomFilterValidatedProgram does. 3. Texmap can reuse many custom filter classes without huge refactoring. I support the third reason. createCustomFilterEffect() in FilterEffectRenderer makes CustomFilterValidatedProgram and FECustomFilter from CustomFilterOperation. FECustomFilter plays a all role to render css shaders. I think TexMap reuse FECustomFilter to render css shaders because TexMap can have CustomFilterProgram though we need to refactor FECustomFilter little. In conclusion, if we choose the second solution, we can enable css shaders in TexMap reusing existing custom filter classes. If you don't mind, I want to enable css shaders in TexMap soon.

Thank you for the explanation! How about we guard all this with USE(COORDINATED_GRAPHICS), and move it out of WebCoreArgumentCoders.[h|cpp] tp WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.[h|cpp], This way there would be no confusion about what this is used for. Andersca, any issues with this?

(In reply to comment #11) > Thank you for the explanation! > How about we guard all this with USE(COORDINATED_GRAPHICS), and move it out of WebCoreArgumentCoders.[h|cpp] tp WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.[h|cpp], It's good idea. I can do this in this bug.

Created attachment 167967 [details] Patch

Comment on attachment 167967 [details] Patch Please rename StillCustomFilterProgram to WebCustomFilterProgram.

Created attachment 168539 [details] Patch

(In reply to comment #14) > (From update of attachment 167967 [details]) > Please rename StillCustomFilterProgram to WebCustomFilterProgram. Done. I filed Bug 98990 to enable CSS Shaders in TexMap. If you don't mind, I want to enable css shaders in TexMap. I'm afraid that I filed that bug by myself thought your team or you may be working on now.

Comment on attachment 168539 [details] Patch Please use CoordinatedGraphicsArgumentCoders and not CoordinatedArgumentCoders (only noticed that now)

(In reply to comment #16) > (In reply to comment #14) > > (From update of attachment 167967 [details] [details]) > > Please rename StillCustomFilterProgram to WebCustomFilterProgram. > > Done. > > I filed Bug 98990 to enable CSS Shaders in TexMap. If you don't mind, I want to enable css shaders in TexMap. I'm afraid that I filed that bug by myself thought your team or you may be working on now. Nobody is currently working on it. If you get it running it would be awesome!

Created attachment 168595 [details] Patch

(In reply to comment #18) > Nobody is currently working on it. If you get it running it would be awesome! Thanks! I'm very glad that I can contribute useful patches to TexMap soon. (In reply to comment #17) > (From update of attachment 168539 [details]) > Please use CoordinatedGraphicsArgumentCoders and not CoordinatedArgumentCoders (only noticed that now) Oops. I'm sorry for causing this redundant iteration.

Comment on attachment 168595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168595&action=review Great! Please fix nit-picks before committing... > Source/WebKit2/ChangeLog:10 > + Coordinated Graphics will use this css shaders serialization when it enables CSS > + Shaders. Please remove these lines. > Source/WebKit2/ChangeLog:30 > + WebCustomFilterProgram is made from serialized data of > + StyleCustomFilterProgram in UI Process. UI Process uses this class > + instead of StyleCustomFilterProgram because UI Process can not get a > + custom filter program via network. Just say "WebCustomFilterProgram is made to serialize the data of a StyleCustomFilterProgram to the UI process." > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29 > +#if USE(COORDINATED_GRAPHICS) Put this line after the main include > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:150 > + encoder->encode(parameters[i]->name()); > + encoder->encodeEnum(parameters[i]->parameterType()); Please save parameters[i] in a local variable.

> Oops. I'm sorry for causing this redundant iteration. Thank you for helping with developing this feature! It's highly appreciated. Iterations are absolutely welcome as long as you're open to feedback :)

Comment on attachment 168595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168595&action=review >> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29 >> +#if USE(COORDINATED_GRAPHICS) > > Put this line after the main include Do I mean like following code? I don't understand fully what do you say. #include "config.h" #include "CoordinatedGraphicsArgumentCoders.h" #if USE(COORDINATED_GRAPHICS) These code are similar to GraphicsContext3D. #include "config.h" #if USE(3D_GRAPHICS) #include "GraphicsContext3D.h"

Created attachment 168619 [details] Patch

Comment on attachment 168595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168595&action=review >> Source/WebKit2/ChangeLog:10 >> + Shaders. > > Please remove these lines. Done. >> Source/WebKit2/ChangeLog:30 >> + custom filter program via network. > > Just say "WebCustomFilterProgram is made to serialize the data of a StyleCustomFilterProgram to the UI process." Done. Thanks for rephrasing >>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29 >>> +#if USE(COORDINATED_GRAPHICS) >> >> Put this line after the main include > > Do I mean like following code? I don't understand fully what do you say. > #include "config.h" > #include "CoordinatedGraphicsArgumentCoders.h" > #if USE(COORDINATED_GRAPHICS) > > > These code are similar to GraphicsContext3D. > #include "config.h" > > #if USE(3D_GRAPHICS) > > #include "GraphicsContext3D.h" Done as I understood. Check please. >> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:150 >> + encoder->encodeEnum(parameters[i]->parameterType()); > > Please save parameters[i] in a local variable. Done.

(In reply to comment #22) > Thank you for helping with developing this feature! It's highly appreciated. Iterations are absolutely welcome as long as you're open to feedback :) Thank you! and I think I'm open :)

Comment on attachment 168619 [details] Patch Looks good, but patch doesn't apply on EWS

Comment on attachment 168619 [details] Patch Looks fine, but patch doesn't apply...

Created attachment 168625 [details] Patch

(In reply to comment #28) > (From update of attachment 168619 [details]) > Looks fine, but patch doesn't apply... I don't know what's happen. I post the patch again.

Created attachment 168668 [details] Patch

Comment on attachment 168625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168625&action=review > Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterProgram.h:61 > + , m_fragmentShaderString(m_fragmentShaderString) There is typo to initialize m_fragmentShaderString from m_fragmentShaderString. It is why I posted new patch.

Created attachment 168670 [details] Patch

Created attachment 169382 [details] Patch

(In reply to comment #34) > Created an attachment (id=169382) [details] > Patch Rebase upstream.

Comment on attachment 169382 [details] Patch Clearing flags on attachment: 169382 Committed r131741: <http://trac.webkit.org/changeset/131741>

All reviewed patches have been landed. Closing bug.

(In reply to comment #36) > (From update of attachment 169382 [details]) > Clearing flags on attachment: 169382 > > Committed r131741: <http://trac.webkit.org/changeset/131741> It broke the Qt --minimal build: /ramdisk/qt-linux-release-minimal/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66: error: 'WebKit' is not a namespace-name /ramdisk/qt-linux-release-minimal/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66: error: expected namespace-name before ';' token Could you fix it, please?

after commenting the using namespace line, I got one more error: /home/oszi/WebKit/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:260: error: 'WebCustomFilterProgram' has not been declared

(In reply to comment #39) > after commenting the using namespace line, I got one more error: > /home/oszi/WebKit/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:260: error: 'WebCustomFilterProgram' has not been declared I'm sorry. I'll fix ASAP.

(In reply to comment #38) > Could you fix it, please? Thanks for reporting! I filed a Bug 99775 and posted a patch.