Bug 117786 – Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Karol]

According to WebGL's revision 10, extensions should be named: WEBGL_lose_context, WEBGL_depth_texture, WEBGL_compressed_texture_s3tc.

[Karol]

Created attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

[Kalyan]

Comment on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-2414
> -        || equalIgnoringCase(name, "WEBKIT_WEBGL_lose_context")

Is it safe to remove these checks?? Seems blink still has the checks in place.

[Karol]

(In reply to comment #2)
> (From update of attachment 204975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-2414
> > -        || equalIgnoringCase(name, "WEBKIT_WEBGL_lose_context")
> 
> Is it safe to remove these checks?? Seems blink still has the checks in place.

It is clear that it may break some backward-compatibility in web applications that use this extension with WEBKIT_WEBGL_ prefix.

However latest official specification from Khronos http://www.khronos.org/registry/webgl/extensions/ does not mention any WEBKIT_ extensions anymore.

The code should be kept clean and compliant with spec all the time and this backward-compatibility, should be removed. If not now, then when?

[Kalyan]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 204975 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> The code should be kept clean and compliant with spec all the time and this backward-compatibility, should be removed. If not now, then when?

I am sure there is a process in place to handle these kind of issues in WebKit. It's just that I am not so familiar with the process. From the comments it clearly states that we are keeping them around for a certain grace period.

[Karol]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 204975 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> > The code should be kept clean and compliant with spec all the time and this backward-compatibility, should be removed. If not now, then when?
> 
> I am sure there is a process in place to handle these kind of issues in WebKit. It's just that I am not so familiar with the process. From the comments it clearly states that we are keeping them around for a certain grace period.

These comments were made on december 2011 (almost 2 years ago). Isn't the grace period long enough?
What process exactly are you talking about? Do you know anyone we should consult about this?

[Karol]

(In reply to comment #2)
> (From update of attachment 204975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-2414
> > -        || equalIgnoringCase(name, "WEBKIT_WEBGL_lose_context")
> 
> Is it safe to remove these checks?? Seems blink still has the checks in place.

Hey, i dug recently into blink/chrom code, and found out, that they are not using WEBKIT_ prefix anymore:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLLoseContext.cpp&q=WEBGL_lose_context&sq=package:chromium&type=cs&l=79

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLDepthTexture.cpp&q=WEBGL_depth_texture&sq=package:chromium&type=cs

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLCompressedTextureS3TC.cpp&q=WEBGL_compressed_texture_s3tc&sq=package:chromium&type=cs

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 204975 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> > 
> > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-2414
> > > -        || equalIgnoringCase(name, "WEBKIT_WEBGL_lose_context")
> > 
> > Is it safe to remove these checks?? Seems blink still has the checks in place.
> 
> Hey, i dug recently into blink/chrom code, and found out, that they are not using WEBKIT_ prefix anymore:
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLLoseContext.cpp&q=WEBGL_lose_context&sq=package:chromium&type=cs&l=79
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLDepthTexture.cpp&q=WEBGL_depth_texture&sq=package:chromium&type=cs
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/canvas/WebGLCompressedTextureS3TC.cpp&q=WEBGL_compressed_texture_s3tc&sq=package:chromium&type=cs

This is inaccurate I believe, take a look at WebGLRenderingContext::ExtensionTracker::matchesNameWithPrefixes(). The code was merely refactored.

[Kalyan]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (From update of attachment 204975 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> > > The code should be kept clean and compliant with spec all the time and this backward-compatibility, should be removed. If not now, then when?
> > 
> > I am sure there is a process in place to handle these kind of issues in WebKit. It's just that I am not so familiar with the process. From the comments it clearly states that we are keeping them around for a certain grace period.
> 
> These comments were made on december 2011 (almost 2 years ago). Isn't the grace period long enough?
> What process exactly are you talking about? Do you know anyone we should consult about this?

As said, I am not familiar with how we should decide to do these kind of changes (which effect backward compatibility etc). Sending a mail to webkit mailing list might help here.

[Kalyan]

(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > (From update of attachment 204975 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=204975&action=review
> As said, I am not familiar with how we should decide to do these kind of changes (which effect backward compatibility etc). Sending a mail to webkit mailing list might help here.

LGTM.

https://lists.webkit.org/pipermail/webkit-dev/2013-July/025127.html

[Dean Jackson]

Comment on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

Thanks!

[WebKit Commit Bot]

Comment on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

Rejecting attachment 204975 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 204975, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/994753

[Karol]

(In reply to comment #10)
> (From update of attachment 204975 [details])
> Thanks!

hi dean, commit-bot says there is OOPS! (no reviewer put in changelog)
can i put you name there?

[Chris Dumez]

Comment on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by (OOPS!! NOBODY).

It is because this line is badly formatted. Add Dean's name here and reupload. Then request only cq, not review.

[Karol]

Created attachment 206371 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

the old one didn't have reviewer put inside
attached new patch

[WebKit Commit Bot]

Comment on attachment 206371 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

Clearing flags on attachment: 206371

Committed r152527: <http://trac.webkit.org/changeset/152527>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.