Summary: | CanvasSurface should go away (there is no OffscreenCanvas) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, darin, eric, levin, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
James Robinson
2010-07-09 16:50:20 PDT
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. Created attachment 61119 [details]
Patch
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/webkit-dev@lists.webkit.org/msg07080.html it seems I should just ignore it. 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 (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. Attachment 61119 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3518004 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! Created attachment 61126 [details]
File local constants instead of statics, fix initialization order
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> All reviewed patches have been landed. Closing bug. 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 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. 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 |