Bug 35292 - [Qt] Need a final integration between QtLauncher and QGVLauncher
Summary: [Qt] Need a final integration between QtLauncher and QGVLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-23 06:46 PST by Jesus Sanchez-Palencia
Modified: 2010-02-24 11:30 PST (History)
5 users (show)

See Also:


Attachments
Clone Window feature (4.01 KB, patch)
2010-02-23 07:02 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Remove QGVLauncher (19.96 KB, patch)
2010-02-23 07:03 PST, Jesus Sanchez-Palencia
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Clone Window feature - v2 (4.00 KB, patch)
2010-02-23 07:11 PST, Jesus Sanchez-Palencia
kenneth: review-
Details | Formatted Diff | Diff
Clone Window feature - v3 (4.47 KB, patch)
2010-02-23 10:14 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Coding Style fixes (2.06 KB, patch)
2010-02-23 10:33 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2010-02-23 06:46:23 PST
The last missing feature on QtLauncher is to be able to clone the window.
After this we can remove QGVLauncher from the source tree.

Patches are coming.
Comment 1 Jesus Sanchez-Palencia 2010-02-23 07:02:56 PST
Created attachment 49290 [details]
Clone Window feature

Strongly based on the old code from QGVLauncher.
Comment 2 WebKit Review Bot 2010-02-23 07:03:45 PST
Attachment 49290 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/QtLauncher/main.cpp:163:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/QtLauncher/main.cpp:188:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jesus Sanchez-Palencia 2010-02-23 07:03:53 PST
Created attachment 49291 [details]
Remove QGVLauncher

This patch should land only after the clone window patch.
Comment 4 Jesus Sanchez-Palencia 2010-02-23 07:11:03 PST
Created attachment 49294 [details]
Clone Window feature - v2

style-check fixes.
Comment 5 Kenneth Rohde Christiansen 2010-02-23 07:14:29 PST
Comment on attachment 49294 [details]
Clone Window feature - v2


> -    LauncherWindow(QString url = QString());
> +    LauncherWindow(QString url = QString(), QGraphicsScene* scene = 0);

Doing two constructors seems better, with a private ::init() method.
Comment 6 Antonio Gomes 2010-02-23 07:33:39 PST
jeez, we also try to avoid this kind of thing:

-    initializeView();
+
+    // if there is a Scene, we should enable QGraphicsView mode
+    if (scene == 0)
+        initializeView();
+    else
+        initializeView(true);


bool parameter is contextless
Comment 7 Jesus Sanchez-Palencia 2010-02-23 10:14:41 PST
Created attachment 49300 [details]
Clone Window feature - v3

Now with 2 constructors and a private init. :)
Comment 8 Jesus Sanchez-Palencia 2010-02-23 10:33:54 PST
Created attachment 49303 [details]
Coding Style fixes

check-webkit-style was complaining about these lines.
Comment 9 WebKit Commit Bot 2010-02-23 12:39:36 PST
Comment on attachment 49300 [details]
Clone Window feature - v3

Clearing flags on attachment: 49300

Committed r55163: <http://trac.webkit.org/changeset/55163>
Comment 10 WebKit Commit Bot 2010-02-23 12:47:44 PST
Comment on attachment 49291 [details]
Remove QGVLauncher

Rejecting patch 49291 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 2
Last 500 characters of output:
--------------------
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit.pro
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/QGVLauncher/QGVLauncher.pro
rm 'WebKit/qt/QGVLauncher/QGVLauncher.pro'
patching file WebKit/qt/QGVLauncher/main.cpp
rm 'WebKit/qt/QGVLauncher/main.cpp'
Could not open 'WebKit/qt/QGVLauncher' to list files: 0 at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 314, <> line 648.

Full output: http://webkit-commit-queue.appspot.com/results/299977
Comment 11 WebKit Commit Bot 2010-02-23 13:13:43 PST
Comment on attachment 49303 [details]
Coding Style fixes

Clearing flags on attachment: 49303

Committed r55164: <http://trac.webkit.org/changeset/55164>
Comment 12 Jesus Sanchez-Palencia 2010-02-23 13:40:47 PST
(In reply to comment #10)

Eric, do you know what went wrong here?

> (From update of attachment 49291 [details])
> Rejecting patch 49291 from commit-queue.
> 
> Failed to run
> "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 2
> Last 500 characters of output:
> --------------------
> patching file ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file WebKit.pro
> patching file WebKit/qt/ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file WebKit/qt/QGVLauncher/QGVLauncher.pro
> rm 'WebKit/qt/QGVLauncher/QGVLauncher.pro'
> patching file WebKit/qt/QGVLauncher/main.cpp
> rm 'WebKit/qt/QGVLauncher/main.cpp'
> Could not open 'WebKit/qt/QGVLauncher' to list files: 0 at
> /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 314, <>
> line 648.
> 
> Full output: http://webkit-commit-queue.appspot.com/results/299977

Thanks
Comment 13 Eric Seidel (no email) 2010-02-23 14:09:56 PST
svn-apply must be having trouble with your patch.  Possibly for moving directories or some other unsupported feature of svn-apply.
Comment 14 Jesus Sanchez-Palencia 2010-02-24 11:30:36 PST
Patches landed, bug closed.