Closed Bug 973815 Opened 6 years ago Closed 6 years ago

WebGL EXT_blend_minmax may now be implemented

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: pyalot, Assigned: guillaume.abadie)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

The WebGL extension EXT_blend_minmax is now in draft and may be implemented: http://www.khronos.org/registry/webgl/extensions/EXT_blend_minmax/
Since I've implemented it in WebGL 2, taking it!
Assignee: nobody → guillaume.abadie
Attached patch ext_blend_minmax.patch (obsolete) — Splinter Review
This patch implements the WebGL extension EXT_blend_minmax passing WebGL conformance 1.0.3 tests.

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=2beffa2cceb0
Attachment #8436442 - Flags: review?(jgilbert)
Forgotten to add content/canvas/src/WebGLExtensionBlendMinMax.cpp... Might be useful in the patch! ^^

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=c7b755d5e201
Attachment #8436442 - Attachment is obsolete: true
Attachment #8436442 - Flags: review?(jgilbert)
Attachment #8436451 - Flags: review?(jgilbert)
Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

Review of attachment 8436451 [details] [diff] [review]:
-----------------------------------------------------------------

BZ: We're adding more WG-approved IDL.
Attachment #8436451 - Flags: review?(jgilbert)
Attachment #8436451 - Flags: review?(bzbarsky)
Attachment #8436451 - Flags: review+
Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

r=me
Attachment #8436451 - Flags: review?(bzbarsky) → review+
Component: General → Graphics
Product: Firefox → Core
Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

Review of attachment 8436451 [details] [diff] [review]:
-----------------------------------------------------------------

BZ: We're adding more WG-approved IDL.

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +159,5 @@
>              return WebGLExtensionDrawBuffers::IsSupported(this);
>          case WebGLExtensionID::EXT_frag_depth:
>              return WebGLExtensionFragDepth::IsSupported(this);
> +        case WebGLExtensionID::EXT_blend_minmax:
> +            return WebGLExtensionBlendMinMax::IsSupported(this);

Actually, I need to you put this in the Draft Extension list.
Attachment #8436451 - Flags: review+ → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Graphics → Canvas: WebGL
Your are very lucky that inbound was closed last night when I tried to push! =)
Attachment #8438061 - Flags: review?(jgilbert)
Comment on attachment 8438061 [details] [diff] [review]
ext_blend_minmax.patch - revision 3

Review of attachment 8438061 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8438061 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 8438061 [details] [diff] [review]
> ext_blend_minmax.patch - revision 3
> 
> Review of attachment 8438061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!

Pleasure is all mine. But I couldn't land it... I had this message:

remote: ************************** ERROR ****************************
remote: 
remote: WebIDL file dom/webidl/WebGLRenderingContext.webidl altered in changeset 67d859377462 without DOM peer review
remote: 
remote: 
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: 
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************

But my commit message is:
Bug 973815 - Implements WebGL extension EXT_blend_minmax - r=jgilbert, bzbarsky 

What did I do wrong?
Flags: needinfo?(bzbarsky)
The space after the ','?  ehsan, what does the hook actually match?
Flags: needinfo?(bzbarsky) → needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] from comment #12)
> The space after the ','?  ehsan, what does the hook actually match?

It looks for all r=... tokens separated by whitespace and then gets the part after the equal sign and breaks it on the comma character and looks at each token.  For example here the hook sees "r=jgilbert," -> "jgilbert," -> ["jgilbert"], and the "bzbarsky" part is completely invisible to it.

Can you please fix your commit message to drop that whitespace?  I see no sane way of making the hook accept this commit message without adding footguns to it.  Sorry!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #12)
> > The space after the ','?  ehsan, what does the hook actually match?
> 
> It looks for all r=... tokens separated by whitespace and then gets the part
> after the equal sign and breaks it on the comma character and looks at each
> token.  For example here the hook sees "r=jgilbert," -> "jgilbert," ->
> ["jgilbert"], and the "bzbarsky" part is completely invisible to it.
> 
> Can you please fix your commit message to drop that whitespace?  I see no
> sane way of making the hook accept this commit message without adding
> footguns to it.  Sorry!

Thanks Ehsan! =)
Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a6e235e674

I suspect we must have a trailing "using namespace mozilla:gl;" somewhere in a header file when not building to B2G, but I couldn't find it...
It's probably in a .cpp file and propagating across to other files due to unified builds.
https://hg.mozilla.org/mozilla-central/rev/d5a6e235e674
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.