Bug 51947

Summary: Define GC3D types to match GL types and use them in WebGraphicsContext3D
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, enne, evan, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 45557    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch kbr: review+

Zhenyao Mo
Reported 2011-01-05 13:11:14 PST
We need to re-define GC3D types again in WebGraphicsContext3D, same as in GraphicsContext3D. Landing this will take some extra effort because it needs to be synced in webkit and chromium.
Attachments
Patch (17.14 KB, patch)
2011-01-25 18:16 PST, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Patch (17.45 KB, patch)
2011-01-26 16:19 PST, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Patch (17.55 KB, patch)
2011-01-26 16:25 PST, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Patch (26.03 KB, patch)
2011-01-27 11:06 PST, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Patch (26.12 KB, patch)
2011-01-27 17:02 PST, Zhenyao Mo 		kbr: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Zhenyao Mo
Comment 1 2011-01-05 17:10:35 PST
*** Bug 51964 has been marked as a duplicate of this bug. ***
Zhenyao Mo
Comment 2 2011-01-24 14:32:07 PST
Also, remove sizeInBytes().
Zhenyao Mo
Comment 3 2011-01-25 18:16:47 PST
Created attachment 80157 [details] Patch
WebKit Review Bot
Comment 4 2011-01-25 18:18:34 PST
Attachment 80157 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebGraphicsContext3D.h:149: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:177: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:178: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:287: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:289: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:304: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Russell
Comment 5 2011-01-26 15:15:35 PST
Comment on attachment 80157 [details] Patch Could you merge this with https://bugs.webkit.org/show_bug.cgi?id=53154 which just landed?
Zhenyao Mo
Comment 6 2011-01-26 16:19:10 PST
Created attachment 80258 [details] Patch
WebKit Review Bot
Comment 7 2011-01-26 16:21:48 PST
Attachment 80258 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebGraphicsContext3D.h:147: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:179: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:180: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:289: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:291: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:306: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zhenyao Mo
Comment 8 2011-01-26 16:25:09 PST
Created attachment 80259 [details] Patch
Zhenyao Mo
Comment 9 2011-01-26 16:25:55 PST
I'll compile one more time before landing to make sure it works fine.
WebKit Review Bot
Comment 10 2011-01-26 16:26:36 PST
Attachment 80259 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebGraphicsContext3D.h:149: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:181: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:182: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:291: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:293: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:308: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 11 2011-01-26 16:32:08 PST
Attachment 80258 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7525361
Kenneth Russell
Comment 12 2011-01-26 16:35:06 PST
Comment on attachment 80259 [details] Patch r=me. Please let this pass through the EWS bots before landing it.
Zhenyao Mo
Comment 13 2011-01-27 11:06:27 PST
Created attachment 80350 [details] Patch
WebKit Review Bot
Comment 14 2011-01-27 11:08:18 PST
Attachment 80350 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebGraphicsContext3D.h:149: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:181: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:182: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:291: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:293: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:308: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zhenyao Mo
Comment 15 2011-01-27 11:09:11 PST
Just realized that because I duplicated the code in chromium side, I don't need to have two copies of the code here: I only need to define USE_WGC3D_TYPES (so the chromium side could switch to use WGC3D types) and directly change the types in each function. This patch only removes the duplicated copy that uses the generic types (as comparing with the previous r+'d patch).
WebKit Review Bot
Comment 16 2011-01-27 11:12:02 PST
Attachment 80350 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7550365
Zhenyao Mo
Comment 17 2011-01-27 11:14:47 PST
Please ignore the chromium bots build failure: it hasn't picked up my chromium side CL yet. Will only land this patch when chromium rev is rolled over.
WebKit Review Bot
Comment 18 2011-01-27 12:05:12 PST
Attachment 80350 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7576376
Kenneth Russell
Comment 19 2011-01-27 13:47:08 PST
Comment on attachment 80350 [details] Patch The Chromium build failures are real. You'll need to land the Chromium side patch first and update chromium_rev in Source/WebKit/chromium/DEPS to prevent this patch from breaking the build.
Zhenyao Mo
Comment 20 2011-01-27 13:52:22 PST
I know that. That's why I said I will wait until chromium rev roll over my chromium side cl (or I will do it myself if no one does that). Please review again.
Kenneth Russell
Comment 21 2011-01-27 14:02:27 PST
(In reply to comment #20) > I know that. That's why I said I will wait until chromium rev roll over my chromium side cl (or I will do it myself if no one does that). > > Please review again. I need to stress again that this patch will need to contain an update to Source/WebKit/chromium/DEPS. There's no point in reviewing it again until the Chromium side lands, at which point you can put up a new patch that contains the DEPS roll and which will build successfully on the EWS bots.
Zhenyao Mo
Comment 22 2011-01-27 14:06:52 PST
Chromium side already landed. I prefer to roll webkit chromium rev independently so if some test failure happens, it will be clear it's from the roll or it's from my patch. If no one did by the end of the day, then I'll do it. As for testing this patch, I build it locally so it should be safe.
Kenneth Russell
Comment 23 2011-01-27 14:29:06 PST
(In reply to comment #22) > Chromium side already landed. I prefer to roll webkit chromium rev independently so if some test failure happens, it will be clear it's from the roll or it's from my patch. > > If no one did by the end of the day, then I'll do it. Rolling forward the Chromium version in DEPS is usually pretty safe. I think you should feel free to either submit another patch doing it so you can get this all landed faster, or do it in the same patch. In the recent revision history, people have been rolling forward DEPS simultaneously with WebKit changes requiring a roll. > As for testing this patch, I build it locally so it should be safe. Understood, but the EWS bots are there for a reason.
Zhenyao Mo
Comment 24 2011-01-27 15:54:37 PST
Just my lucky day. Tried two revision, both failed compiling. Keep trying...
Zhenyao Mo
Comment 25 2011-01-27 17:02:28 PST
Created attachment 80379 [details] Patch same patch, trigger the trybots after chromium rev roll
WebKit Review Bot
Comment 26 2011-01-27 17:04:40 PST
Attachment 80379 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebGraphicsContext3D.h:154: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:186: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:187: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:296: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:298: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/public/WebGraphicsContext3D.h:313: The parameter name "w" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zhenyao Mo
Comment 27 2011-01-27 17:20:07 PST
Please review. The chromium linux bot is green, since this patch has no platform specific code, so it should be safe to land now.
Kenneth Russell
Comment 28 2011-01-27 17:22:37 PST
Comment on attachment 80379 [details] Patch Thanks for persisting with this patch. Looks good.
Zhenyao Mo
Comment 29 2011-01-27 17:29:38 PST
Committed r76876: <http://trac.webkit.org/changeset/76876>
Note You need to log in before you can comment on or make changes to this bug.