Bug 117786

Summary: Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_
Product: WebKit Reporter: Karol <k.swiniarsk2>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, dino, esprehn+autocc, kalyan.kondapally, kbr, noam, rwlbuis, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_
dino: review+, commit-queue: commit-queue-
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_ none

Description Karol 2013-06-19 01:42:26 PDT
According to WebGL's revision 10, extensions should be named: WEBGL_lose_context, WEBGL_depth_texture, WEBGL_compressed_texture_s3tc.
Comment 1 Karol 2013-06-19 01:46:05 PDT
Created attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_
Comment 2 Kalyan 2013-06-25 01:18:10 PDT
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.
Comment 3 Karol 2013-06-25 02:38:25 PDT
(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?
Comment 4 Kalyan 2013-06-25 06:50:27 PDT
(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.
Comment 5 Karol 2013-06-26 07:01:59 PDT
(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?
Comment 7 Chris Dumez 2013-06-26 07:29:06 PDT
(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.
Comment 8 Kalyan 2013-06-26 12:16:58 PDT
(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.
Comment 9 Kalyan 2013-07-02 06:06:36 PDT
(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
Comment 10 Dean Jackson 2013-07-09 16:08:45 PDT
Comment on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_

Thanks!
Comment 11 WebKit Commit Bot 2013-07-09 16:10:39 PDT
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
Comment 12 Karol 2013-07-09 23:42:19 PDT
(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?
Comment 13 Chris Dumez 2013-07-09 23:47:21 PDT
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.
Comment 14 Karol 2013-07-10 00:11:44 PDT
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
Comment 15 WebKit Commit Bot 2013-07-10 00:42:36 PDT
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>
Comment 16 WebKit Commit Bot 2013-07-10 00:42:40 PDT
All reviewed patches have been landed.  Closing bug.