Bug 67172

Created attachment 105563 [details] Patch

Comment on attachment 105563 [details] Patch Why are you renaming this to private?

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.

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

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

Created attachment 105638 [details] Patch

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?

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

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

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.

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.

Created attachment 105773 [details] Build Fix for GTK + Style fix On my system, r94083 broke building WebGL enabled GTK. The patch solves the problem.

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.

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

Reopening for GTK breakage

Comment on attachment 105773 [details] Build Fix for GTK + Style fix Clearing flags on attachment: 105773 Committed r94328: <http://trac.webkit.org/changeset/94328>