Bug 61802 – [Qt][WK2] Add QGLWidget viewport support to MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Viatcheslav Ostapenko]

Add command line parameter and menu item to MiniBrowser application enabling use of QGLWidget for browser viewport.

[Viatcheslav Ostapenko]

Created attachment 95472 [details]
Add QGLWidget viewport mode to minibrowser app.

[WebKit Review Bot]

Attachment 95472 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/qt/B..." exit_code: 1

Tools/MiniBrowser/qt/BrowserView.cpp:48:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 95476 [details]
Fix style problem

[Simon Hausmann]

Comment on attachment 95476 [details]
Fix style problem

View in context: https://bugs.webkit.org/attachment.cgi?id=95476&action=review

r=me

At some point we should probably make that simply the default...

> Tools/MiniBrowser/qt/BrowserView.cpp:49
> +        setViewport(new QGLWidget());

Nitpick, here you use new GLWidget() and further down just "new QGLWidget"

> Tools/MiniBrowser/qt/BrowserView.cpp:76
> +void BrowserView::toggleGLViewport(bool useQGLWidgetViewport)
> +{
> +#if defined(QT_CONFIGURED_WITH_OPENGL)
> +    setViewport(useQGLWidgetViewport ? new QGLWidget : 0);
> +#endif
> +}

setViewport is a public method, so this code doesn't actually have to live in BrowserView but could also be done from the outside.

[Viatcheslav Ostapenko]

(In reply to comment #4)
> (From update of attachment 95476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95476&action=review
> 
> r=me
> 
> At some point we should probably make that simply the default...
> 
> > Tools/MiniBrowser/qt/BrowserView.cpp:49
> > +        setViewport(new QGLWidget());
> 
> Nitpick, here you use new GLWidget() and further down just "new QGLWidget"
> 
> > Tools/MiniBrowser/qt/BrowserView.cpp:76
> > +void BrowserView::toggleGLViewport(bool useQGLWidgetViewport)
> > +{
> > +#if defined(QT_CONFIGURED_WITH_OPENGL)
> > +    setViewport(useQGLWidgetViewport ? new QGLWidget : 0);
> > +#endif
> > +}
> 
> setViewport is a public method, so this code doesn't actually have to live in BrowserView but could also be done from the outside.

Ooops! It's already committed.
Should I make those changes as separate patch?

[Viatcheslav Ostapenko]

Removed from commit queue to address some issues mentioned by Simon.

[Viatcheslav Ostapenko]

Created attachment 95504 [details]
Fix issues from Simon review.

[Andreas Kling]

Comment on attachment 95504 [details]
Fix issues from Simon review.

LGTM

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95504 [details]:

http/tests/websocket/tests/frame-length-longer-than-buffer.html bug 61837 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95504 [details]
Fix issues from Simon review.

Clearing flags on attachment: 95504

Committed r87824: <http://trac.webkit.org/changeset/87824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.