Bug 42005 – CanvasSurface should go away (there is no OffscreenCanvas)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

CanvasSurface should go away (there is no OffscreenCanvas)

[James Robinson]

http://trac.webkit.org/changeset/55201 introduced a new base class for HTMLCanvasElement called CanvasSurface.  The intention was that this would allow for code sharing with the then-proposed OffscreenCanvas.  However, there is no OffscreenCanvas and there's unlikely to be one soon.  Additionally CanvasSurface breaks encapsulation pretty badly by doing "static_cast<HTMLCanvasElement* const>(this)".  Until an abstraction is really needed we should just use HTMLCanvasElement when we want to talk about a canvas.

[James Robinson]

Created attachment 61119 [details]
Patch

[James Robinson]

This patch updates all of the in-tree build systems, but I'm not really sure what to do about WebCore/WebCore.order.  It references a number of functions on CanvasSurface, some of which are now on HTMLCanvasElement and some of which are now dead.  According to http://www.mail-archive.com/[e-mail]/msg07080.html it seems I should just ignore it.

[Darin Adler]

Comment on attachment 61119 [details]
Patch

> +    static const float MaxCanvasArea;
> +    static const int DefaultWidth;
> +    static const int DefaultHeight;

We normally use file-local constants instead of constant static data members.

r=me

[Darin Adler]

(In reply to comment #4)
> (From update of attachment 61119 [details])
> > +    static const float MaxCanvasArea;
> > +    static const int DefaultWidth;
> > +    static const int DefaultHeight;
> 
> We normally use file-local constants instead of constant static data members.

Unless the constants are needed outside the file.

[Eric Seidel (no email)]

Attachment 61119 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3518004

[James Robinson]

Compile failed due to initialization/declaration order mismatch.  I'll fix that and switch to file statics as those constants are now only referenced in one file (HTMLCanvasElement.cpp).

Thanks for the super-duper fast review, Darin!

[James Robinson]

Created attachment 61126 [details]
File local constants instead of statics, fix initialization order

[James Robinson]

Comment on attachment 61126 [details]
File local constants instead of statics, fix initialization order

Clearing flags on attachment: 61126

Committed r63025: <http://trac.webkit.org/changeset/63025>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63025 might have broken Qt Linux Release minimal
The following changes are on the blame list:
http://trac.webkit.org/changeset/63024
http://trac.webkit.org/changeset/63025

[James Robinson]

Yup, was me.  I think this patch depends on some transitive includes to pick up SECURITY_ERR that doesn't exist on all platforms.  Will fix promptly.

[James Robinson]

http://trac.webkit.org/changeset/63031

[James Robinson]

That patch fixed Qt Linux Release, Qt Windows is now failing but with an internal compiler error unrelated to my patch:

g++ -c -Wall -Wextra -Wreturn-type -fno-strict-aliasing -Wcast-align -Wchar-subscripts -Wformat-security -Wreturn-type -Wno-unused-parameter -Wno-sign-compare -Wno-switch -Wno-switch-enum -Wundef -Wmissing-noreturn -Winit-self -O3 -frtti -fexceptions -mthreads -DUNICODE -DQT_LARGEFILE_SUPPORT -DBUILDING_QT__=1 -DWTF_USE_ACCELERATED_COMPOSITING -DUSE_SYSTEM_MALLOC -DNDEBUG -D_HAS_TR1=0 -DBUILDING_QT__ -DBUILDING_JavaScriptCore -DBUILDING_WTF -DQT_NO_DEBUG -DQT_CORE_LIB -DQT_THREAD_SUPPORT -I"c:\Qt\2010.02\qt\include\QtCore" -I"c:\Qt\2010.02\qt\include" -I"..\..\..\JavaScriptCore" -I"..\..\..\..\build" -I"..\..\..\JavaScriptCore\assembler" -I"..\..\..\JavaScriptCore\bytecode" -I"..\..\..\JavaScriptCore\bytecompiler" -I"..\..\..\JavaScriptCore\debugger" -I"..\..\..\JavaScriptCore\interpreter" -I"..\..\..\JavaScriptCore\jit" -I"..\..\..\JavaScriptCore\parser" -I"..\..\..\JavaScriptCore\pcre" -I"..\..\..\JavaScriptCore\profiler" -I"..\..\..\JavaScriptCore\runtime" -I"..\..\..\JavaScriptCore\wtf" -I"..\..\..\JavaScriptCore\wtf\symbian" -I"..\..\..\JavaScriptCore\wtf\unicode" -I"..\..\..\JavaScriptCore\yarr" -I"..\..\..\JavaScriptCore\API" -I"..\..\..\JavaScriptCore\ForwardingHeaders" -I"generated" -I"..\include\QtWebKit" -I"..\..\..\JavaScriptCore\pcre" -I"t:\WindowsRelease\buildslave\qt-windows-32bit-release\build\WebKitBuild\Release\JavaScriptCore\tmp" -I"c:\Qt\2010.02\qt\include\ActiveQt" -I"." -I"..\..\..\JavaScriptCore" -I"." -I"c:\Qt\2010.02\qt\mkspecs\default" -o obj\release\JSGlobalObjectFunctions.o ..\..\..\JavaScriptCore\runtime\JSGlobalObjectFunctions.cpp
..\..\..\JavaScriptCore\runtime\ArrayPrototype.cpp: In function 'void* JSC::arrayProtoFuncJoin(JSC::ExecState*)':
..\..\..\JavaScriptCore\runtime\ArrayPrototype.cpp:263:30: internal compiler error: Segmentation fault
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make.exe[1]: *** [obj/release/ArrayPrototype.o] Error 1
make.exe[1]: *** Waiting for unfinished jobs....
make.exe[1]: Leaving directory `T:/WindowsRelease/buildslave/qt-windows-32bit-release/build/WebKitBuild/Release/JavaScriptCore'
make: *** [sub-JavaScriptCore-make_default-ordered] Error 2