Bug 181228 – Web Inspector: [GTK] Copy copies to nowhere

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Dan Jacobson]

Please see https://bugzilla.gnome.org/show_bug.cgi?id=792133

Package: epiphany-browser
Version: 3.26.5.1-1

-- System Information:
Debian Release: buster/sid
  APT prefers experimental
  APT policy: (990, 'experimental'), (500, 'unstable')
Architecture: amd64 (x86_64)

Kernel: Linux 4.14.0-2-amd64 (SMP w/2 CPU cores)
Locale: LANG=zh_TW.UTF-8, LC_CTYPE=zh_TW.UTF-8 (charmap=UTF-8), LANGUAGE=en_US:en (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash
Init: systemd (via /run/systemd/system)

Versions of packages epiphany-browser depends on:
ii  dbus-x11 [dbus-session-bus]  1.12.2-1
ii  epiphany-browser-data        3.26.5.1-1
ii  gsettings-desktop-schemas    3.24.1-2
ii  iso-codes                    3.77-1
ii  libc6                        2.26-0experimental3
ii  libcairo2                    1.15.8-3
ii  libgcr-base-3-1              3.20.0-6
ii  libgcr-ui-3-1                3.20.0-6
ii  libgdk-pixbuf2.0-0           2.36.11-1
ii  libglib2.0-0                 2.54.2-5
ii  libgmp10                     2:6.1.2+dfsg-1.1
ii  libgnome-desktop-3-12        3.26.2-4
ii  libgtk-3-0                   3.22.26-2
ii  libhogweed4                  3.4-1
ii  libicu57                     57.1-8
ii  libjavascriptcoregtk-4.0-18  2.19.3-1
ii  libjson-glib-1.0-0           1.4.2-3
ii  libnettle6                   3.4-1
ii  libnotify4                   0.7.7-3
ii  libpango-1.0-0               1.40.14-1
ii  libsecret-1-0                0.18.5-5
ii  libsoup2.4-1                 2.60.2-2
ii  libsqlite3-0                 3.21.0-1
ii  libwebkit2gtk-4.0-37         2.19.3-1
ii  libxml2                      2.9.4+dfsg1-6.1

Versions of packages epiphany-browser recommends:
pn  browser-plugin-evince      <none>
ii  ca-certificates            20170717
pn  evince                     <none>
ii  libwebkit2gtk-4.0-37-gtk2  2.19.3-1
pn  yelp                       <none>

epiphany-browser suggests no packages.

-- no debconf information

[Michael Catanzaro]

Please leave some useful comment when reporting a bug. The problem is that it's impossible to Copy anything out of the web inspector; the Copy context menu item does nothing. I presume it's left unimplemented somewhere in the web inspector code, as if it used the WebCore pasteboard classes it ought to work.

[Carlos Garcia Campos]

This regressed when we decided to complicate the code to use GMenus instead of GtkAction. The thing is that those items are actually submenu items, with options, for example in the case of Copy to copy HTML, Text, XPath, etc. We are not correctly handling submenus when populating the context menu received from the web process.

[Carlos Garcia Campos]

Created attachment 366482 [details]
Patch

[Michael Catanzaro]

Comment on attachment 366482 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:158
> +        case ActionType:
> +        case CheckableActionType: {

Is there no complaint from -Wimplicit-fallthrough? We have a FALLTHROUGH macro that expands to a suitable attribute to avoid warnings here:

case ActionType:
    FALLTHROUGH;
case CheckableActionType: {

[Joseph Pecoraro]

Comment on attachment 366482 [details]
Patch

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

>> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:158
>> +        case CheckableActionType: {
> 
> Is there no complaint from -Wimplicit-fallthrough? We have a FALLTHROUGH macro that expands to a suitable attribute to avoid warnings here:
> 
> case ActionType:
>     FALLTHROUGH;
> case CheckableActionType: {

FALLTHROUGH is normally only necessary if there has been some statement between case statements. Here they are grouped directly next to each other so it probably won't be necessary.

[Devin Rousso]

Comment on attachment 366482 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:126
> +            Vector<WebContextMenuItemGlib> subMenuItems;
> +            populateSubMenu(itemData, subMenuItems);
> +            items.append(WebContextMenuItemGlib(itemData, WTFMove(subMenuItems)));

Rather than pass `subMenuItems` in as a modifiable arguments, could you have `populateSubMenu` return the `Vector<WebContextMenuItemGlib>` instead?
```
    Vector<WebContextMenuItemGlib> WebContextMenuProxyGtk::populateSubMenu(const WebContextMenuItemData& subMenuItemData)
    {
        Vector<WebContextMenuItemGlib> items;
        for (const auto& itemData : subMenuItemData.submenu()) {
            if (itemData.type() == SubmenuType)
                items.append(WebContextMenuItemGlib(itemData, populateSubMenu(itemData));
            else
                items.append(itemData);
        }
        return items;
    }
```

> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:153
> +            Vector<WebContextMenuItemGlib> subMenuItems;
> +            populateSubMenu(item->data(), subMenuItems);
> +            WebContextMenuItemGlib menuitem(item->data(), WTFMove(subMenuItems));

Ditto (>124).

[Carlos Garcia Campos]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 366482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366482&action=review
> 
> >> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:158
> >> +        case CheckableActionType: {
> > 
> > Is there no complaint from -Wimplicit-fallthrough? We have a FALLTHROUGH macro that expands to a suitable attribute to avoid warnings here:
> > 
> > case ActionType:
> >     FALLTHROUGH;
> > case CheckableActionType: {
> 
> FALLTHROUGH is normally only necessary if there has been some statement
> between case statements. Here they are grouped directly next to each other
> so it probably won't be necessary.

Exactly, FALLTHROUGH is for the cases where it's not obvious if you want to fall through or you forgot to add the break. That's not the case here.

[Carlos Garcia Campos]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 366482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366482&action=review
> 
> > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:126
> > +            Vector<WebContextMenuItemGlib> subMenuItems;
> > +            populateSubMenu(itemData, subMenuItems);
> > +            items.append(WebContextMenuItemGlib(itemData, WTFMove(subMenuItems)));
> 
> Rather than pass `subMenuItems` in as a modifiable arguments, could you have
> `populateSubMenu` return the `Vector<WebContextMenuItemGlib>` instead?
> ```
>     Vector<WebContextMenuItemGlib>
> WebContextMenuProxyGtk::populateSubMenu(const WebContextMenuItemData&
> subMenuItemData)
>     {
>         Vector<WebContextMenuItemGlib> items;
>         for (const auto& itemData : subMenuItemData.submenu()) {
>             if (itemData.type() == SubmenuType)
>                 items.append(WebContextMenuItemGlib(itemData,
> populateSubMenu(itemData));
>             else
>                 items.append(itemData);
>         }
>         return items;
>     }
> ```

Yes, much better. Thanks!

> > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:153
> > +            Vector<WebContextMenuItemGlib> subMenuItems;
> > +            populateSubMenu(item->data(), subMenuItems);
> > +            WebContextMenuItemGlib menuitem(item->data(), WTFMove(subMenuItems));
> 
> Ditto (>124).

[Carlos Garcia Campos]

Committed r243793: <https://trac.webkit.org/changeset/243793>

[Radar WebKit Bug Importer]

<rdar://problem/49551554>