Bug 48335 - [GTK] [review] Support for new-window in GtkLauncher
Summary: [GTK] [review] Support for new-window in GtkLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-26 08:46 PDT by Nicolas Dufresne
Modified: 2010-10-29 09:32 PDT (History)
3 users (show)

See Also:


Attachments
Enable popup window in GtkLauncher (10.17 KB, patch)
2010-10-26 08:46 PDT, Nicolas Dufresne
mrobinson: review-
Details | Formatted Diff | Diff
Enable popup window in GtkLauncher (with changelog) (11.66 KB, patch)
2010-10-29 08:22 PDT, Nicolas Dufresne
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Dufresne 2010-10-26 08:46:48 PDT
Created attachment 71894 [details]
Enable popup window in GtkLauncher

The GtkLauncher application does not support opening new window when a "target=_blank" is clicked or similar call to window.open() is made withing a user gesture. Instead, GtkLauncher does nothing. This prevents using GtkLauncher for testing certain sites. The attach patch implement minimal support for creating new window. This is done by implementing signals "create-web-view", "web-view-ready" and "close-web-view".
Comment 1 Martin Robinson 2010-10-28 13:19:48 PDT
Comment on attachment 71894 [details]
Enable popup window in GtkLauncher

Great patch. I'm in favor of putting a little more into GtkLauncher while keeping it minimal. This patch does need a ChangeLog though, which can be generated via prepare-ChangeLog in the WebKitTools/Scripts directory.
Comment 2 Martin Robinson 2010-10-28 13:21:35 PDT
CCing some other GTK+ reviewers, who might have more insight into whether this should be added to GtkLauncher. I'm all for it though.
Comment 3 Xan Lopez 2010-10-28 16:36:57 PDT
Yes. I'm also in favor of keeping the launcher (very) minimal, but this seems to be needed to browse to some pages since we do nothing in the default handler of create-web-view. Perhaps that's something we should fix too, not sure.
Comment 4 Martin Robinson 2010-10-28 16:47:19 PDT
(In reply to comment #3)
> Yes. I'm also in favor of keeping the launcher (very) minimal, but this seems to be needed to browse to some pages since we do nothing in the default handler of create-web-view. Perhaps that's something we should fix too, not sure.

I think two good metrics to help decide whether or not to put something in the GTKLauncher are:

1. Will this help debug layout test failures?
2. Will this help demonstrate the API to embedders?

In my mind, this patch does both.
Comment 5 Nicolas Dufresne 2010-10-29 08:22:26 PDT
Created attachment 72336 [details]
Enable popup window in GtkLauncher (with changelog)

The GtkLauncher application does not support opening new window when
a link with "target=_blank" is clicked or similar call to
window.open(). Instead, GtkLauncher does nothing which breaks
navigation of some websites.
Comment 6 Martin Robinson 2010-10-29 08:51:44 PDT
Comment on attachment 72336 [details]
Enable popup window in GtkLauncher (with changelog)

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

> WebKitTools/ChangeLog:11
> +        Enable popup window in GtkLauncher
> +
> +        The GtkLauncher application does not support opening new window when
> +        a link with "target=_blank" is clicked or similar call to
> +        window.open(). Instead, GtkLauncher does nothing which breaks
> +        navigation of some websites.
> +        https://bugs.webkit.org/show_bug.cgi?id=48335

It's more typical for the order to be:
<bug title>
<bug url>

<description>

This isn't a big deal though, and I'll fix it locally before landing.
Comment 7 Xan Lopez 2010-10-29 08:59:16 PDT
(In reply to comment #6)
> It's more typical for the order to be:
> <bug title>
> <bug url>
> 
> <description>
> 
> This isn't a big deal though, and I'll fix it locally before landing.

prepare-ChangeLog --bug <number> gets it right for you at no extra cost! :)
Comment 8 Martin Robinson 2010-10-29 09:32:09 PDT
Committed r70886: <http://trac.webkit.org/changeset/70886>