Bug 71870 – Rename WEBKIT_lose_context to WEBKIT_WEBGL_lose_context

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 115495 [details]
Patch

[WebKit Review Bot]

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.

[Kenneth Russell]

(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?

[Kenneth Russell]

Created attachment 115500 [details]
Patch

[James Robinson]

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

[Kenneth Russell]

Created attachment 115507 [details]
Patch

[Kenneth Russell]

(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.

[James Robinson]

Comment on attachment 115507 [details]
Patch

R=me

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 118163 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 118163 [details]
Patch

Attachment 118163 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10751030

[Kenneth Russell]

Created attachment 118283 [details]
Patch

Fixed GTK build problem

[James Robinson]

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?

[James Robinson]

Comment on attachment 118283 [details]
Patch

Otherwise, this code looks fine.

[Kenneth Russell]

(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.

[Kenneth Russell]

Committed r102418: <http://trac.webkit.org/changeset/102418>

[Kenichi Ishibashi]

Reverted r102418 for reason:

Caused chromium build failure

Committed r102430: <http://trac.webkit.org/changeset/102430>

[Kenneth Russell]

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.

[Kenneth Russell]

I've built this patch on Chromium Windows as well with no problems. Re-landing it.

[Kenneth Russell]

Committed r102973: <http://trac.webkit.org/changeset/102973>