Bug 71870 - Rename WEBKIT_lose_context to WEBKIT_WEBGL_lose_context
Summary: Rename WEBKIT_lose_context to WEBKIT_WEBGL_lose_context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on: 74161
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-08 17:56 PST by Kenneth Russell
Modified: 2011-12-15 11:54 PST (History)
11 users (show)

See Also:


Attachments
Patch (38.92 KB, patch)
2011-11-16 17:53 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (39.25 KB, patch)
2011-11-16 18:08 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (38.33 KB, patch)
2011-11-16 18:28 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (49.27 KB, patch)
2011-12-06 19:52 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (50.09 KB, patch)
2011-12-07 15:35 PST, Kenneth Russell
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-11-08 17:56:59 PST
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.
Comment 1 Kenneth Russell 2011-11-16 17:53:30 PST
Created attachment 115495 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-16 17:56:01 PST
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.
Comment 3 Kenneth Russell 2011-11-16 18:04:50 PST
(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?
Comment 4 Kenneth Russell 2011-11-16 18:08:12 PST
Created attachment 115500 [details]
Patch
Comment 5 James Robinson 2011-11-16 18:13:04 PST
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
Comment 6 Kenneth Russell 2011-11-16 18:28:09 PST
Created attachment 115507 [details]
Patch
Comment 7 Kenneth Russell 2011-11-16 18:29:10 PST
(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 8 James Robinson 2011-11-16 18:56:39 PST
Comment on attachment 115507 [details]
Patch

R=me
Comment 9 Kenneth Russell 2011-12-06 19:36:17 PST
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.
Comment 10 Kenneth Russell 2011-12-06 19:52:40 PST
Created attachment 118163 [details]
Patch
Comment 11 Gustavo Noronha (kov) 2011-12-06 20:01:23 PST
Comment on attachment 118163 [details]
Patch

Attachment 118163 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10751030
Comment 12 Kenneth Russell 2011-12-07 15:35:09 PST
Created attachment 118283 [details]
Patch

Fixed GTK build problem
Comment 13 James Robinson 2011-12-07 17:17:33 PST
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 14 James Robinson 2011-12-07 17:33:48 PST
Comment on attachment 118283 [details]
Patch

Otherwise, this code looks fine.
Comment 15 Kenneth Russell 2011-12-07 17:38:25 PST
(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.
Comment 16 Kenneth Russell 2011-12-08 19:07:23 PST
Committed r102418: <http://trac.webkit.org/changeset/102418>
Comment 17 Kenichi Ishibashi 2011-12-08 21:23:52 PST
Reverted r102418 for reason:

Caused chromium build failure

Committed r102430: <http://trac.webkit.org/changeset/102430>
Comment 18 Kenneth Russell 2011-12-11 18:48:59 PST
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.
Comment 19 Kenneth Russell 2011-12-15 09:53:08 PST
I've built this patch on Chromium Windows as well with no problems. Re-landing it.
Comment 20 Kenneth Russell 2011-12-15 11:54:32 PST
Committed r102973: <http://trac.webkit.org/changeset/102973>