Bug 36175

Summary: [Qt] Make QtLaucher able to select the ViewportUpdateMode
Product: WebKit Reporter: Diego Gonzalez <diegohcg>
Component: WebKit Misc.Assignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: benjamin, jesus, kenneth, tonikitoo
Priority: P5 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 35303    
Attachments:
Description Flags
Proposed patch
kenneth: review-
(committed in r56117) Proposed patch none

Description Diego Gonzalez 2010-03-16 11:06:44 PDT
Provide to QtLauncher a way to change the ViewPortUpdateMode
when it's in graphics based mode.
Comment 1 Diego Gonzalez 2010-03-16 11:14:41 PDT
Created attachment 50811 [details]
Proposed patch
Comment 2 Kenneth Rohde Christiansen 2010-03-16 11:28:08 PDT
Comment on attachment 50811 [details]
Proposed patch


> +void LauncherWindow::selectViewportUpdateMode(int mode)

select is not a really good name for this. Consider using "change" or "set"


> +{
> +    if (!isGraphicsBased())
> +        return;

I guess you should update the gViewportUpdateMode anyway, so you are bailing out a bit too early

> +
> +    gViewportUpdateMode = QGraphicsView::ViewportUpdateMode(mode);
> +    WebViewGraphicsBased* view = static_cast<WebViewGraphicsBased*>(m_view);
> +    view->setViewportUpdateMode(gViewportUpdateMode);
> +}
Comment 3 Diego Gonzalez 2010-03-16 12:53:36 PDT
Created attachment 50827 [details]
(committed in r56117) Proposed patch

Changed according Kenneth's suggestions
Comment 4 Benjamin Poulain 2010-03-17 03:03:58 PDT
Setting the status of the task so it does not appear unassigned.


I might misunderstand the purpose of the patch but for me this should not go in QtLauncher.

If the objective is to compare performance, you should create a reduction in the webkit benchmarks. Definitely not update the launcher.
If the objective is to find bugs, you should add autotests in graphicsview.

Why do you need that in the launcher?
Comment 5 Jesus Sanchez-Palencia 2010-03-17 07:16:37 PDT
(In reply to comment #4)
> Why do you need that in the launcher?

QtLauncher is becoming a more complete test tool of QtWebKit. And we are focusing on mobile tests (Symbian and Maemo).

We are following https://bugs.webkit.org/show_bug.cgi?id=35303 requirements, some ideas from Simon and others, to have all command line options as menu options on the Launcher. With this it will become easier to test QtWebKit on Symbian devices.
Comment 6 Benjamin Poulain 2010-03-17 07:35:31 PDT
(In reply to comment #5)
> We are following https://bugs.webkit.org/show_bug.cgi?id=35303 requirements,

This bug does not say why such options would be required, neither why it is specific to Symbian.
Comment 7 Jesus Sanchez-Palencia 2010-03-17 07:52:45 PDT
(In reply to comment #6)
> This bug does not say why such options would be required, neither why it is
> specific to Symbian.

Yes, that is true. My guess is that No'am is mainly working with Symbian debugging and that is why he opened the bug. But I don't think that those wanted features are Symbian specific, that is why we are leaving them available for any platform/OS.
Comment 8 Antonio Gomes 2010-03-17 10:38:26 PDT
Comment on attachment 50827 [details]
(committed in r56117) Proposed patch

the ChangeLog was wrong =/

Fixed, and landed manually in r56117