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+

Description Zhenyao Mo 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.
Comment 1 Zhenyao Mo 2011-01-05 17:10:35 PST
*** Bug 51964 has been marked as a duplicate of this bug. ***
Comment 2 Zhenyao Mo 2011-01-24 14:32:07 PST
Also, remove sizeInBytes().
Comment 3 Zhenyao Mo 2011-01-25 18:16:47 PST
Created attachment 80157 [details]
Patch
Comment 4 WebKit Review Bot 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.
Comment 5 Kenneth Russell 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?
Comment 6 Zhenyao Mo 2011-01-26 16:19:10 PST
Created attachment 80258 [details]
Patch
Comment 7 WebKit Review Bot 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.
Comment 8 Zhenyao Mo 2011-01-26 16:25:09 PST
Created attachment 80259 [details]
Patch
Comment 9 Zhenyao Mo 2011-01-26 16:25:55 PST
I'll compile one more time before landing to make sure it works fine.
Comment 10 WebKit Review Bot 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.
Comment 11 WebKit Review Bot 2011-01-26 16:32:08 PST
Attachment 80258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7525361
Comment 12 Kenneth Russell 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.
Comment 13 Zhenyao Mo 2011-01-27 11:06:27 PST
Created attachment 80350 [details]
Patch
Comment 14 WebKit Review Bot 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.
Comment 15 Zhenyao Mo 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).
Comment 16 WebKit Review Bot 2011-01-27 11:12:02 PST
Attachment 80350 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7550365
Comment 17 Zhenyao Mo 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.
Comment 18 WebKit Review Bot 2011-01-27 12:05:12 PST
Attachment 80350 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7576376
Comment 19 Kenneth Russell 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.
Comment 20 Zhenyao Mo 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.
Comment 21 Kenneth Russell 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.
Comment 22 Zhenyao Mo 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.
Comment 23 Kenneth Russell 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.
Comment 24 Zhenyao Mo 2011-01-27 15:54:37 PST
Just my lucky day.  Tried two revision, both failed compiling.  Keep trying...
Comment 25 Zhenyao Mo 2011-01-27 17:02:28 PST
Created attachment 80379 [details]
Patch

same patch, trigger the trybots after chromium rev roll
Comment 26 WebKit Review Bot 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.
Comment 27 Zhenyao Mo 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.
Comment 28 Kenneth Russell 2011-01-27 17:22:37 PST
Comment on attachment 80379 [details]
Patch

Thanks for persisting with this patch. Looks good.
Comment 29 Zhenyao Mo 2011-01-27 17:29:38 PST
Committed r76876: <http://trac.webkit.org/changeset/76876>