Mozilla indicated that they desired to add the WEBKIT_lose_context extension to their WebGL implementation for testing purposes. After discussion on the public_webgl mailing list it was decided to simply rename this extension to WEBGL_EXT_lose_context to avoid going down the route of having one browser vendor implement another's WebGL extensions. This renaming has been done in the extension registry and needs to be done in WebKit.
Created attachment 115495 [details] Patch
Attachment 115495 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/html/canvas/WebGLExtLoseContext.h:26: #ifndef header guard has wrong style, please use: WebGLExtLoseContext_h [build/header_guard] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 115495 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 > > Source/WebCore/html/canvas/WebGLExtLoseContext.h:26: #ifndef header guard has wrong style, please use: WebGLExtLoseContext_h [build/header_guard] [5] > Total errors found: 1 in 23 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Shouldn't "webkit-patch upload" have caught this?
Created attachment 115500 [details] Patch
Comment on attachment 115500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115500&action=review There are a number of tests checked into the chromium repository that use this extension name. There also might be content in the wild using this identifier. Should we perhaps recognize both for a period of time to give people a chance to update their code? > Source/WebCore/html/canvas/WebGLExtLoseContext.h:47 > + WebKitLoseContext(WebGLRenderingContext*); explicit please > Source/WebCore/html/canvas/WebGLExtLoseContext.h:47 > + WebGLExtLoseContext(WebGLRenderingContext*); explicit please > Tools/Scripts/do-webcore-rename:100 > + "WebKitLoseContext" => "WebGLExtLoseContext", you shouldn't check this bit in
Created attachment 115507 [details] Patch
(In reply to comment #5) > (From update of attachment 115500 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115500&action=review > > There are a number of tests checked into the chromium repository that use this extension name. There also might be content in the wild using this identifier. Should we perhaps recognize both for a period of time to give people a chance to update their code? Fair enough. Done. > > Source/WebCore/html/canvas/WebGLExtLoseContext.h:47 > > + WebKitLoseContext(WebGLRenderingContext*); > > explicit please Done. > > Tools/Scripts/do-webcore-rename:100 > > + "WebKitLoseContext" => "WebGLExtLoseContext", > > you shouldn't check this bit in In my experience the convention is to check in the change to do-webcore-rename along with the changes it made, but I'm happy enough to not do so. Removed.
Comment on attachment 115507 [details] Patch R=me
After much more discussion on the WebGL mailing list the decision was made to use vendor prefixes for draft extensions being implemented by multiple browsers. Updated the summary.
Created attachment 118163 [details] Patch
Comment on attachment 118163 [details] Patch Attachment 118163 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10751030
Created attachment 118283 [details] Patch Fixed GTK build problem
Comment on attachment 118283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118283&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2583 > + result.append("WEBKIT_WEBGL_lose_context"); this appears to be a (slight) WebGL spec violation: DOMString[ ] getSupportedExtensions() Returns an array of all the supported extension strings. Any string in this list, when passed to getExtension must return a valid object. Any other string passed to getExtension must return null. object getExtension(DOMString name) Returns an object if the passed extension is supported, or null if not. The object returned from getExtension contains any constants or functions used by the extension, if any. A returned object may have no constants or functions if the extension does not define any, but a unique object must still be returned. That object is used to indicate that the extension has been enabled. We're not exporting WEBKIT_lose_context in getSupportedExtensions() but we are returning non-null to getExtension("WEBKIT_lose_context"). If this is a deliberate choice to make migration easier I think it sounds fine, but I want to make sure it's a conscious violation and not an accident. > LayoutTests/ChangeLog:6 > + Synchronized context-lost-restored.html and context-lost.html with do we want to attempt to verify that WEBKIT_lost_context keeps working until we decide to remove it?
Comment on attachment 118283 [details] Patch Otherwise, this code looks fine.
(In reply to comment #13) > (From update of attachment 118283 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118283&action=review > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2583 > > + result.append("WEBKIT_WEBGL_lose_context"); > > this appears to be a (slight) WebGL spec violation: > DOMString[ ] getSupportedExtensions() > Returns an array of all the supported extension strings. Any string in this list, when passed to getExtension must return a valid object. Any other string passed to getExtension must return null. > object getExtension(DOMString name) > Returns an object if the passed extension is supported, or null if not. The object returned from getExtension contains any constants or functions used by the extension, if any. A returned object may have no constants or functions if the extension does not define any, but a unique object must still be returned. That object is used to indicate that the extension has been enabled. > > We're not exporting WEBKIT_lose_context in getSupportedExtensions() but we are returning non-null to getExtension("WEBKIT_lose_context"). If this is a deliberate choice to make migration easier I think it sounds fine, but I want to make sure it's a conscious violation and not an accident. Yes, this is a slight violation, but it doesn't seem to be a good idea to continue advertising the deprecated name. No applications I know of are actually calling getSupportedExtensions(), only getExtension(). I'd remove support for it right now if it weren't for the concern you raised earlier about possible breakage of Chromium tests. Once those have been updated I am going to remove support for the old name. > > LayoutTests/ChangeLog:6 > > + Synchronized context-lost-restored.html and context-lost.html with > > do we want to attempt to verify that WEBKIT_lost_context keeps working until we decide to remove it? No, this would be a waste of code.
Committed r102418: <http://trac.webkit.org/changeset/102418>
Reverted r102418 for reason: Caused chromium build failure Committed r102430: <http://trac.webkit.org/changeset/102430>
The bug ID with the failing build log was https://bugs.webkit.org/show_bug.cgi?id=74161 . It seems clear that the bot simply needed to be clobbered. The Chromium build worked fine on my local workstation.
I've built this patch on Chromium Windows as well with no problems. Re-landing it.
Committed r102973: <http://trac.webkit.org/changeset/102973>