Summary: | Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_ | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Karol <k.swiniarsk2> | ||||||
Component: | WebGL | Assignee: | 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
Karol
2013-06-19 01:42:26 PDT
Created attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_
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. (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? (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. (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? (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 (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. (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. (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 on attachment 204975 [details]
Change recently ratified extensions prefixes from WEBKIT_WEBGL_ to WEBGL_
Thanks!
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 (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 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. 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 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> All reviewed patches have been landed. Closing bug. |