Bug 67172 – Rename GraphicsContext3DInternal to GraphicsContext3DPrivate and add a dummy version of this class for Mac

• Component: Canvas
• Status: RESOLVED CONFIGURATION CHANGED

[Chris Marrin]

Rename GraphicsContext3DInternal to GraphicsContext3DPrivate and add a dummy version of this class for Mac

[Chris Marrin]

Created attachment 105563 [details]
Patch

[Darin Adler]

Comment on attachment 105563 [details]
Patch

Why are you renaming this to private?

[WebKit Review Bot]

Attachment 105563 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:61:  The parameter name "hostWindow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:66:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:116:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:117:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:118:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:195:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:210:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:212:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:214:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:216:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:218:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:220:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:222:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:224:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:225:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:226:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:227:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:274:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 18 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 105563 [details]
Patch

Attachment 105563 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9567272

[Chris Marrin]

(In reply to comment #2)
> (From update of attachment 105563 [details])
> Why are you renaming this to private?

It is part of a bigger restructuring of GraphicsContext3D (https://bugs.webkit.org/show_bug.cgi?id=66903), which is being done to make it easier to extend the class to other platforms. GC3D is getting really messy with ifdefs, and a EGL support is being added (https://bugs.webkit.org/show_bug.cgi?id=59064). So this will make it a lot easier to do all this work. 

This change is being done to match other patterns in the code, e.g., AnimationController.

[Chris Marrin]

Created attachment 105638 [details]
Patch

[Kenneth Russell]

Comment on attachment 105638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105638&action=review

Looks good. r=me

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68
> +    GraphicsContext3D* m_graphicsContext3D;

Explicitly comment that this is weak?

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 105563 [details] [details])
> > Why are you renaming this to private?
> 
> It is part of a bigger restructuring of GraphicsContext3D (https://bugs.webkit.org/show_bug.cgi?id=66903), which is being done to make it easier to extend the class to other platforms.

But how is “private” more accurate than “internal”? I think the existing name is fine. But maybe there is a good reason to rename it that I am not aware of.

[Chris Marrin]

Committed r94083: <http://trac.webkit.org/changeset/94083>

[Dominik Röttsches (drott)]

Created attachment 105768 [details]
Build Fix for GTK

On my system, r94083 broke building WebGL enabled GTK. The patch solves this problem. I generated it from my git commit, not sure if this format is acceptable.

[WebKit Review Bot]

Attachment 105768 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/gtk/GraphicsContext3DPrivate.cpp:21:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gtk/GraphicsContext3DPrivate.cpp:22:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominik Röttsches (drott)]

Created attachment 105773 [details]
Build Fix for GTK + Style fix

On my system, r94083 broke building WebGL enabled GTK. The patch solves the problem.

[Chris Marrin]

Comment on attachment 105773 [details]
Build Fix for GTK + Style fix

This patch isn't ideal. Generally, you get a delete, a new file (like you have here) and then a diff on that new file showing all the renamed functions. The way you've done it, you can't see the actual changes to the new file. But I'll trust that this is what you've done. So r=me. 

I'm confused about why the ews bots didn't catch this mistake.

[Chris Marrin]

(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (From update of attachment 105563 [details] [details] [details])
> > > Why are you renaming this to private?
> > 
> > It is part of a bigger restructuring of GraphicsContext3D (https://bugs.webkit.org/show_bug.cgi?id=66903), which is being done to make it easier to extend the class to other platforms.
> 
> But how is “private” more accurate than “internal”? I think the existing name is fine. But maybe there is a good reason to rename it that I am not aware of.

This is something we talked about on #webkit with the Google guys. We agreed that it followed the same conventions as other parts of the code. I needed to make Mac use the private class so I did it at the same time.

[Chris Marrin]

Reopening for GTK breakage

[WebKit Review Bot]

Comment on attachment 105773 [details]
Build Fix for GTK + Style fix

Clearing flags on attachment: 105773

Committed r94328: <http://trac.webkit.org/changeset/94328>