Bug 51947 – Define GC3D types to match GL types and use them in WebGraphicsContext3D

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

[Zhenyao Mo]

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.

[Zhenyao Mo]

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

[Zhenyao Mo]

Also, remove sizeInBytes().

[Zhenyao Mo]

Created attachment 80157 [details]
Patch

[WebKit Review Bot]

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 on attachment 80157 [details]
Patch

Could you merge this with https://bugs.webkit.org/show_bug.cgi?id=53154 which just landed?

[Zhenyao Mo]

Created attachment 80258 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 80259 [details]
Patch

[Zhenyao Mo]

I'll compile one more time before landing to make sure it works fine.

[WebKit Review Bot]

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]

Attachment 80258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7525361

[Kenneth Russell]

Comment on attachment 80259 [details]
Patch

r=me. Please let this pass through the EWS bots before landing it.

[Zhenyao Mo]

Created attachment 80350 [details]
Patch

[WebKit Review Bot]

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]

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]

Attachment 80350 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7550365

[Zhenyao Mo]

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]

Attachment 80350 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7576376

[Kenneth Russell]

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]

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]

(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]

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]

(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]

Just my lucky day.  Tried two revision, both failed compiling.  Keep trying...

[Zhenyao Mo]

Created attachment 80379 [details]
Patch

same patch, trigger the trybots after chromium rev roll

[WebKit Review Bot]

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]

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 on attachment 80379 [details]
Patch

Thanks for persisting with this patch. Looks good.

[Zhenyao Mo]

Committed r76876: <http://trac.webkit.org/changeset/76876>