Bug 29095 – Rename 3D Canvas related classes to use WebGL prefix

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

This is the official name of the object in the spec.

[Kenneth Russell]

Created attachment 42731 [details]
Patch

To track the naming convention in the WebGL spec, renamed CanvasRenderingContext3D, CanvasObject and subclasses, CanvasArrayBuffer, and CanvasArray and subclasses to use the WebGL prefix.

Updated WebGL tests (both manual and LayoutTests).

Built and tested WebKit in Debug and Release mode. Built and tested Chromium in Debug mode with WebGL enabled. A checkin is needed to the Chromium tree once the WebKit changes have been committed.

[Kenneth Russell]

*** Bug 31170 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

Comment on attachment 42731 [details]
Patch

The normal model for changes like these is to rename the class, then moves the files.  It makes it easier to see what is actually changing.

[Oliver Hunt]

Ken, I've been told that you disagree with my review, however you have made no comments in this bug, nor have you attempted to contact me.  I will try to make this clearer

From the point of view of reviewing a patch, a file move with changes is incredibly difficult.  Therefore we do rename patches in two parts -- the first part is to rename the class, the second is to rename the file.  This means the first patch is simply a whole pile of isolated renames, and the second patch is reported as a move.  This makes reviewing far simpler, and makes it easier to see what is happening in the revision history.

[Kenneth Russell]

I'll resubmit the patch containing only the changes to the files' contents. I'll need that patch committed before I can do the file renamings.

[Kenneth Russell]

Note that splitting this patch in this way will not result in the second patch being a simple move. Because IDL files are changing, references to various headers from the js and v8 bindings will change. I hope that this will not prevent the restructured patch from being approved.

[Oliver Hunt]

(In reply to comment #6)
> Note that splitting this patch in this way will not result in the second patch
> being a simple move. Because IDL files are changing, references to various
> headers from the js and v8 bindings will change. I hope that this will not
> prevent the restructured patch from being approved.

I figured that the IDL files would be a pain to rename/move but they tend to be very small anyway (with the exception of the obvious root context class) -- It may be worth doing the rename+move of Canvas/WebGLRenderingContext separately from everything else

[Kenneth Russell]

There are also interdependencies among the .cpp / .h files in html/canvas, so these will not be a simple renaming either in the second patch.

There is no advantage to renaming CanvasRenderingContext3D in a separate patch. This will only make more work. The majority of the code changes are due to the renaming of the CanvasArray and CanvasObject subclasses.

[Kenneth Russell]

I've tried to split up this patch as requested. I was able to get WebKit to compile and link. It required the addition of several temporary headers like html/canvas/WebGLArray.h so that the derived sources would compile.

I'm having difficulty making Chromium build. The V8 code generation script (specifically, WebCore/WebCore.gyp/scripts/rule_binding.py) assumes that the IDL file's name is the same as the class named in the IDL file.

As I see it, there are a few options:

1) Invest even more time hacking on the V8 code generation scripts.

2) Completely disable the WebGL build for Chromium while this non-atomic renaming is being done.

3) Apply my original, atomic patch.

I don't want to do (1); it is a waste of time.

Oliver, I would prefer to do (3), but if you want, I will disable WebGL in Chromium and upload a new patch to satisfy (2). As I pointed out, this will not result in the second part of the patch being a simple renaming. There are interdependencies between headers like CanvasFloatArray.h and CanvasArray.h which require them to be renamed and updated in a single step. It might be technically possible to do multiple rename / update commits so that no commit contains a renaming of a file that also contains modifications, but this will only prolong the agony of this update.

[Oliver Hunt]

(In reply to comment #9)
> I've tried to split up this patch as requested. I was able to get WebKit to
> compile and link. It required the addition of several temporary headers like
> html/canvas/WebGLArray.h so that the derived sources would compile.
> 
> I'm having difficulty making Chromium build. The V8 code generation script
> (specifically, WebCore/WebCore.gyp/scripts/rule_binding.py) assumes that the
> IDL file's name is the same as the class named in the IDL file.
> 
> As I see it, there are a few options:
> 
> 1) Invest even more time hacking on the V8 code generation scripts.
> 
> 2) Completely disable the WebGL build for Chromium while this non-atomic
> renaming is being done.
> 
> 3) Apply my original, atomic patch.
> 
> I don't want to do (1); it is a waste of time.
> 
> Oliver, I would prefer to do (3), but if you want, I will disable WebGL in
> Chromium and upload a new patch to satisfy (2). As I pointed out, this will not
> result in the second part of the patch being a simple renaming. There are
> interdependencies between headers like CanvasFloatArray.h and CanvasArray.h
> which require them to be renamed and updated in a single step. It might be
> technically possible to do multiple rename / update commits so that no commit
> contains a renaming of a file that also contains modifications, but this will
> only prolong the agony of this update.

*sigh*  this would be immensely easier if chrome didn't choose to have its own bindings.

I am disinclined to make things worse for the normal webkit builds in the name of not destroying chrome builds, but i may be willing to accept an atomic commit after i see what's required to do the rename myself.

[Oliver Hunt]

> *sigh*  this would be immensely easier if chrome didn't choose to have its own
> bindings.
> 
> I am disinclined to make things worse for the normal webkit builds in the name
> of not destroying chrome builds, but i may be willing to accept an atomic
> commit after i see what's required to do the rename myself.

After playing around it would seem that it is difficult to do this degree a renaming without an ancillary typedef.  I don't really like that idea so it would appear that the best solution is an atomic rename and move.

That said, each class can be renamed/moved in a separate patch, rather than a single 1.2mb patch which no one can reasonably review

[Oliver Hunt]

(In reply to comment #11)
> > *sigh*  this would be immensely easier if chrome didn't choose to have its own
> > bindings.
> > 
> > I am disinclined to make things worse for the normal webkit builds in the name
> > of not destroying chrome builds, but i may be willing to accept an atomic
> > commit after i see what's required to do the rename myself.
> 
> After playing around it would seem that it is difficult to do this degree a
> renaming without an ancillary typedef.  I don't really like that idea so it
> would appear that the best solution is an atomic rename and move.
> 
> That said, each class can be renamed/moved in a separate patch, rather than a
> single 1.2mb patch which no one can reasonably review

Okay, just had another attempt and it would seem i can rename all the files trivially and not worry about the types contained therein.  I think the correct approach is therefore to rename the files then rename the classes.  This should produce the nicest set of changes.

[Oliver Hunt]

Maciej has pointed out that we actually have a tool to automatically do renames like this (do-webcore-renames) I'm just seeing if it handles IDL files correctly.  If it does then our historic approach has been to run it and land the patch noting it was done automatically by do-webcore-rename.

If it does i'll just bang through the patches tonight, though you'll need to do v8 binding updates manually i suspect.

[Kenneth Russell]

(In reply to comment #12)
> Okay, just had another attempt and it would seem i can rename all the files
> trivially and not worry about the types contained therein.  I think the correct
> approach is therefore to rename the files then rename the classes.  This should
> produce the nicest set of changes.

I don't understand how this is different from what I've already tried.

 - For the WebKit build, it's still going to be necessary to make some updates to the contents of the files during the renaming. For example, CanvasFloatArray.h moves to WebGLFloatArray.h, but its #include "CanvasArray.h" must be changed to #include "WebGLArray.h".

 - The Chrome build will break in exactly the same way as before because of the mismatch between the IDL file's name and contents.

The patch above can be reviewed in the following way. In the "Formatted Diff" view, each of the moved and edited files shows up three times. First, a complete removal of the old file (e.g. "JSCanvasArrayBufferConstructor.cpp". Second, a complete replacement of the new file (e.g. "JSWebGLArrayBufferConstructor.cpp". Finally, and the only important one, exactly the edits that need to be reviewed, under the name e.g. JSWebGLArrayBufferConstructor.cpp. The terms that were replaced are highlighted in the third version. The first two can be ignored.

The exceptions to this rule in this review are the following files, which were modified:
WebCore/DerivedSources.make
WebCore/WebCore.gypi
WebCore/WebCore.xcodeproj/project.pbxproj
WebCore/bindings/js/JSDOMWindowCustom.cpp
WebCore/bindings/js/JSDocumentCustom.cpp
WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp
WebCore/bindings/scripts/CodeGeneratorV8.pm
WebCore/bindings/v8/DOMObjectsInclude.h
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp
WebCore/bindings/v8/V8DOMWrapper.cpp
WebCore/bindings/v8/V8Index.cpp
WebCore/bindings/v8/V8Index.h
WebCore/bindings/v8/custom/V8CustomBinding.h
WebCore/bindings/v8/custom/V8DocumentCustom.cpp
WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp
WebCore/html/HTMLCanvasElement.cpp
WebCore/manual-tests/webgl/Earth.html
WebCore/manual-tests/webgl/ManyPlanetsDeep.html
WebCore/manual-tests/webgl/SpinningBox.html
WebCore/manual-tests/webgl/TeapotPerPixel.html
WebCore/manual-tests/webgl/TeapotPerVertex.html
WebCore/manual-tests/webgl/resources/CanvasMatrix.js
WebCore/manual-tests/webgl/resources/utils3d.js
WebCore/page/DOMWindow.idl
WebCore/platform/graphics/GraphicsContext3D.h
WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
WebCore/rendering/RenderLayerBacking.cpp
LayoutTests/fast/canvas/webgl/array-unit-tests-expected.txt
LayoutTests/fast/canvas/webgl/array-unit-tests.html
LayoutTests/fast/canvas/webgl/gl-get-calls.html
LayoutTests/fast/canvas/webgl/triangle.html
LayoutTests/fast/canvas/webgl/resources/utils3d.js

I just paged through the review, ignoring all of the files that were completely removed or added, and verifying that the only textual changes were renaming of Canvas to WebGL and dropping of the 3D suffix from CanvasRenderingContext3D. This took less than ten minutes.

[Oliver Hunt]

(In reply to comment #14)
> (In reply to comment #12)
> > Okay, just had another attempt and it would seem i can rename all the files
> > trivially and not worry about the types contained therein.  I think the correct
> > approach is therefore to rename the files then rename the classes.  This should
> > produce the nicest set of changes.
> 
> I don't understand how this is different from what I've already tried.

My implication there was one file at a time.  But also it resulted in a simple patch that did a rename, followed by a vcs move which is trivial to verify across vcs revisions, rather than a move+changes.

>  - For the WebKit build, it's still going to be necessary to make some updates
> to the contents of the files during the renaming. For example,
> CanvasFloatArray.h moves to WebGLFloatArray.h, but its #include "CanvasArray.h"
> must be changed to #include "WebGLArray.h".
> 
>  - The Chrome build will break in exactly the same way as before because of the
> mismatch between the IDL file's name and contents.
>

This seems like a bug in the V8 bindings codegen -- it's probably worth fixing on the assumption we'll hit it again one day, the standard bindings generator script does not have this restriction.
 
> The patch above can be reviewed in the following way. In the "Formatted Diff"
> view, each of the moved and edited files shows up three times. First, a
> complete removal of the old file (e.g. "JSCanvasArrayBufferConstructor.cpp".
> Second, a complete replacement of the new file (e.g.
> "JSWebGLArrayBufferConstructor.cpp". Finally, and the only important one,
> exactly the edits that need to be reviewed, under the name e.g.
> JSWebGLArrayBufferConstructor.cpp. The terms that were replaced are highlighted
> in the third version. The first two can be ignored.

In a 1.2mb file that requires tracking them all down.

> I just paged through the review, ignoring all of the files that were completely
> removed or added, and verifying that the only textual changes were renaming of
> Canvas to WebGL and dropping of the 3D suffix from CanvasRenderingContext3D.
> This took less than ten minutes.
1.2Mb in less than 10 minutes is not a review.

Anyhoo, like i said in my last comment, i'm fairly sure that do-webcore-rename can do the required work automatically, and is something we've used in the past (i'd forgotten about it due to the length of time since we've really needed to rename anything)

--Oliver

[Oliver Hunt]

Okay, in my testing do-webcore-rename updates v8 build scripts as well, so i believe it should be fine to just use it.  I'll start pushing the individual patches in half an hour or so.

[Oliver Hunt]

I have a "big' do-webcore-renames patch that automatically renames all of the canvas/webgl types in one swoop, and the v8 bindings that live in the tree, and the manual-tests :D

Just building to verify.

Alas LayoutTests need to be update manually

[Oliver Hunt]

Created attachment 42852 [details]
Automatic rename patch

This is the important bit of the rename patch.  All renames inside WebCore happen automatically, including manual-test updates.

A few testcases in the LayoutTests directory need to be updated to handle the change in type name.

[Maciej Stachowiak]

Comment on attachment 42852 [details]
Automatic rename patch

r=me

[Oliver Hunt]

Committed r50725