Bug 29826 – Add support for run-time flag for 3D canvas

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

[Kenneth Russell]

In order to enable experimental WebGL support in the nightly builds of the Chromium browser, a run-time flag is needed to cover the functionality. The attached patch adds the necessary infrastructure to the Settings class, and sets the flag to true in the Mac and Windows WebView classes when 3D_CANVAS is enabled in the build.

Built and tested on Mac OS X with --3d-canvas both enabled and disabled to verify no build breakage and that the 3D canvas functionality continues to work as expected.

[Kenneth Russell]

Created attachment 40259 [details]
Patch adding run-time flag covering 3D canvas functionality.

[Chris Marrin]

The use of the WebGL .exp file here is needed in order to allow the runtime flag conditional on the ENABLE_3D_CANVAS compile time flag. You could also have the runtime flag always exist, like we did for the acceleratedCompositingEnabled runtime flag, while the functionality itself is conditionally compiled. That would allow you to get rid of the WebGL .exp file.

[Dimitri Glazkov (Google)]

Comment on attachment 40259 [details]
Patch adding run-time flag covering 3D canvas functionality.

Mostly procedural nits.

> [phone]  Kenneth Russell  <[e-mail]>
> +
> +        Reviewed by [e-mail](?), [e-mail](?).

You probably shouldn't remove the OOPS from here until reviewed. The pre-commit hook makes it your last line of defense.
However -- you don't have a commit bit, so my point is Moooo-t :)

> +
> +        Add support for run-time flag for 3D canvas
> +        https://bugs.webkit.org/show_bug.cgi?id=29826
> +
> +        * DerivedSources.make: Conditionally add
> +	Settings::setExperimentalWebGLEnabled to exported symbol list.
Weird indentation here. Tabs maybe?

> +        * WebCore.WebGL.exp: Added.
> +        * html/HTMLCanvasElement.cpp:
> +        (WebCore::HTMLCanvasElement::getContext): Check page settings for
> +	experimental WebGL flag before returning 3D graphics context.

Ditto.

> +
> +        * WebView/WebView.mm:
> +        (-[WebView _preferencesChangedNotification:]): Enable experimental
> +	WebGL flag when 3D_CANVAS is enabled in the build.

Ditto.

>  
> +#if ENABLE(3D_CANVAS)
> +    settings->setExperimentalWebGLEnabled(true);
> +#endif  // ENABLE(3D_CANVAS)

I didn't realize Win was a supported platform. If it's not, should we do this for all other ports, then?

[Kenneth Russell]

Created attachment 40311 [details]
Revised patch

@Chris Marrin:
> The use of the WebGL .exp file here is needed in order to allow the runtime
> flag conditional on the ENABLE_3D_CANVAS compile time flag. You could also have
> the runtime flag always exist, like we did for the
> acceleratedCompositingEnabled runtime flag, while the functionality itself is
> conditionally compiled. That would allow you to get rid of the WebGL .exp file.

That would simplify both this addition and its eventual removal once the WebGL specification is finalized. Done.

@Dimitri Glazkov:
> You probably shouldn't remove the OOPS from here until reviewed. The pre-commit
> hook makes it your last line of defense.

Done.

> Weird indentation here. Tabs maybe?

Yes, I didn't have tabs-as-spaces set for Change Log mode. Fixed.

> I didn't realize Win was a supported platform. If it's not, should we do this
> for all other ports, then?

Right now Safari only supports WebGL on Mac OS X, but I'm assuming Apple will port it to Windows in the near future. I would prefer to leave the other ports alone since the mechanism by which this flag is set will have to change depending on how the browser vendor wants to expose it.

[Dimitri Glazkov (Google)]

Comment on attachment 40311 [details]
Revised patch

r=me.

[WebKit Commit Bot]

Comment on attachment 40311 [details]
Revised patch

Rejecting patch 40311 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=40311 from bug 29826 failed to download and apply.

[Kenneth Russell]

Created attachment 40317 [details]
Updated patch after merging with trunk

These files changed in trunk yesterday. Updated and merged.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/48893.

[Mark Rowe (bdash)]

There's some weird line breaking and extra parentheses added by this patch.

[Kenneth Russell]

(In reply to comment #9)
> There's some weird line breaking and extra parentheses added by this patch.

Where do you mean?