181228 2018-01-02 16:30:39 -0800 Web Inspector: [GTK] Copy copies to nowhere 2019-04-03 01:26:27 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED https://bugzilla.gnome.org/show_bug.cgi?id=792133 InRadar P2 Normal --- 1 jidanni webkit-unassigned bugs-noreply cgarcia hi inspector-bugzilla-changes joepeck mcatanzaro webkit-bug-importer oldest_to_newest 1385310 0 jidanni 2018-01-02 16:30:39 -0800 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 1385452 1 mcatanzaro 2018-01-03 08:58:23 -0800 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. 1523262 2 cgarcia 2019-04-02 03:38:29 -0700 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. 1523263 3 366482 cgarcia 2019-04-02 03:40:44 -0700 Created attachment 366482 Patch 1523290 4 366482 mcatanzaro 2019-04-02 06:08:54 -0700 Comment on attachment 366482 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: { 1523367 5 366482 joepeck 2019-04-02 11:24:51 -0700 Comment on attachment 366482 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. 1523453 6 366482 hi 2019-04-02 14:05:51 -0700 Comment on attachment 366482 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). 1523641 7 cgarcia 2019-04-03 01:19:21 -0700 (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. 1523642 8 cgarcia 2019-04-03 01:19:49 -0700 (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). 1523643 9 cgarcia 2019-04-03 01:25:33 -0700 Committed r243793: <https://trac.webkit.org/changeset/243793> 1523644 10 webkit-bug-importer 2019-04-03 01:26:27 -0700 <rdar://problem/49551554> 366482 2019-04-02 03:40:44 -0700 2019-04-02 06:08:54 -0700 Patch wi-copy.diff text/plain 4053 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggMjFkZDUwYTgzMTYuLmNlNmY2MzllYzRlIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE5IEBACisyMDE5LTA0LTAyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBXZWIgSW5zcGVjdG9yOiBbR1RLXSBDb3B5IGNvcGllcyB0byBub3do ZXJlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODEy MjgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUg dGhpbmcgaXMgdGhhdCB0aG9zZSBpdGVtcyBhcmUgYWN0dWFsbHkgc3VibWVudSBpdGVtcywgd2l0 aCBvcHRpb25zLCBmb3IgZXhhbXBsZSBpbiB0aGUgY2FzZSBvZiBDb3B5IHRvIGNvcHkKKyAgICAg ICAgSFRNTCwgVGV4dCwgWFBhdGgsIGV0Yy4gV2UgYXJlIG5vdCBjb3JyZWN0bHkgaGFuZGxpbmcg c3VibWVudXMgd2hlbiBwb3B1bGF0aW5nIHRoZSBjb250ZXh0IG1lbnUgcmVjZWl2ZWQgZnJvbQor ICAgICAgICB0aGUgd2ViIHByb2Nlc3MuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvZ3RrL1dlYkNv bnRleHRNZW51UHJveHlHdGsuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJDb250ZXh0TWVudVBy b3h5R3RrOjpwb3B1bGF0ZVN1Yk1lbnUpOiBIZWxwZXIgdG8gcG9wdWxhdGUgc3VibWVudSBpdGVt cyByZWN1cnNpdmVseS4KKyAgICAgICAgKFdlYktpdDo6V2ViQ29udGV4dE1lbnVQcm94eUd0azo6 cG9wdWxhdGUpOiBIYW5kbGUgc3VibWVudSBpdGVtcy4KKyAgICAgICAgKiBVSVByb2Nlc3MvZ3Rr L1dlYkNvbnRleHRNZW51UHJveHlHdGsuaDoKKwogMjAxOS0wNC0wMiAgQ2FybG9zIEdhcmNpYSBD YW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0Nvb3JkaW5hdGVkR3JhcGhp Y3NdIEhpZGRlbiBwYWdlcyBhcmUgbm90IHN1c3BlbmRlZCBhZnRlciBhIHdlYiB2aWV3IHJlc2l6 ZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51 UHJveHlHdGsuY3BwIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51 UHJveHlHdGsuY3BwCmluZGV4IGQyMjAyOWViOGJlLi5iYThlZjc0NmZiMSAxMDA2NDQKLS0tIGEv U291cmNlL1dlYktpdC9VSVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuY3BwCisr KyBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL2d0ay9XZWJDb250ZXh0TWVudVByb3h5R3RrLmNw cApAQCAtMTE3LDYgKzExNywxOCBAQCBHUmVmUHRyPEdNZW51PiBXZWJDb250ZXh0TWVudVByb3h5 R3RrOjpidWlsZE1lbnUoY29uc3QgVmVjdG9yPFdlYkNvbnRleHRNZW51SXRlbQogICAgIHJldHVy biBtZW51OwogfQogCit2b2lkIFdlYkNvbnRleHRNZW51UHJveHlHdGs6OnBvcHVsYXRlU3ViTWVu dShjb25zdCBXZWJDb250ZXh0TWVudUl0ZW1EYXRhJiBzdWJNZW51SXRlbURhdGEsIFZlY3RvcjxX ZWJDb250ZXh0TWVudUl0ZW1HbGliPiYgaXRlbXMpCit7CisgICAgZm9yIChjb25zdCBhdXRvJiBp dGVtRGF0YSA6IHN1Yk1lbnVJdGVtRGF0YS5zdWJtZW51KCkpIHsKKyAgICAgICAgaWYgKGl0ZW1E YXRhLnR5cGUoKSA9PSBTdWJtZW51VHlwZSkgeworICAgICAgICAgICAgVmVjdG9yPFdlYkNvbnRl eHRNZW51SXRlbUdsaWI+IHN1Yk1lbnVJdGVtczsKKyAgICAgICAgICAgIHBvcHVsYXRlU3ViTWVu dShpdGVtRGF0YSwgc3ViTWVudUl0ZW1zKTsKKyAgICAgICAgICAgIGl0ZW1zLmFwcGVuZChXZWJD b250ZXh0TWVudUl0ZW1HbGliKGl0ZW1EYXRhLCBXVEZNb3ZlKHN1Yk1lbnVJdGVtcykpKTsKKyAg ICAgICAgfSBlbHNlCisgICAgICAgICAgICBpdGVtcy5hcHBlbmQoaXRlbURhdGEpOworICAgIH0K K30KKwogdm9pZCBXZWJDb250ZXh0TWVudVByb3h5R3RrOjpwb3B1bGF0ZShjb25zdCBWZWN0b3I8 V2ViQ29udGV4dE1lbnVJdGVtR2xpYj4mIGl0ZW1zKQogewogICAgIEdSZWZQdHI8R01lbnU+IG1l bnUgPSBidWlsZE1lbnUoaXRlbXMpOwpAQCAtMTI4LDEzICsxNDAsMjYgQEAgdm9pZCBXZWJDb250 ZXh0TWVudVByb3h5R3RrOjpwb3B1bGF0ZShjb25zdCBWZWN0b3I8UmVmPFdlYkNvbnRleHRNZW51 SXRlbT4+JiBpdGUKICAgICBHUmVmUHRyPEdNZW51PiBtZW51ID0gYWRvcHRHUmVmKGdfbWVudV9u ZXcoKSk7CiAgICAgR01lbnUqIHNlY3Rpb25NZW51ID0gbWVudS5nZXQoKTsKICAgICBmb3IgKGNv bnN0IGF1dG8mIGl0ZW0gOiBpdGVtcykgewotICAgICAgICBpZiAoaXRlbS0+ZGF0YSgpLnR5cGUo KSA9PSBTZXBhcmF0b3JUeXBlKSB7CisgICAgICAgIHN3aXRjaCAoaXRlbS0+ZGF0YSgpLnR5cGUo KSkgeworICAgICAgICBjYXNlIFNlcGFyYXRvclR5cGU6IHsKICAgICAgICAgICAgIEdSZWZQdHI8 R01lbnU+IHNlY3Rpb24gPSBhZG9wdEdSZWYoZ19tZW51X25ldygpKTsKICAgICAgICAgICAgIGdf bWVudV9hcHBlbmRfc2VjdGlvbihtZW51LmdldCgpLCBudWxscHRyLCBHX01FTlVfTU9ERUwoc2Vj dGlvbi5nZXQoKSkpOwogICAgICAgICAgICAgc2VjdGlvbk1lbnUgPSBzZWN0aW9uLmdldCgpOwot ICAgICAgICB9IGVsc2UgeworICAgICAgICAgICAgYnJlYWs7CisgICAgICAgIH0KKyAgICAgICAg Y2FzZSBTdWJtZW51VHlwZTogeworICAgICAgICAgICAgVmVjdG9yPFdlYkNvbnRleHRNZW51SXRl bUdsaWI+IHN1Yk1lbnVJdGVtczsKKyAgICAgICAgICAgIHBvcHVsYXRlU3ViTWVudShpdGVtLT5k YXRhKCksIHN1Yk1lbnVJdGVtcyk7CisgICAgICAgICAgICBXZWJDb250ZXh0TWVudUl0ZW1HbGli IG1lbnVpdGVtKGl0ZW0tPmRhdGEoKSwgV1RGTW92ZShzdWJNZW51SXRlbXMpKTsKKyAgICAgICAg ICAgIGFwcGVuZChzZWN0aW9uTWVudSwgbWVudWl0ZW0pOworICAgICAgICAgICAgYnJlYWs7Cisg ICAgICAgIH0KKyAgICAgICAgY2FzZSBBY3Rpb25UeXBlOgorICAgICAgICBjYXNlIENoZWNrYWJs ZUFjdGlvblR5cGU6IHsKICAgICAgICAgICAgIFdlYkNvbnRleHRNZW51SXRlbUdsaWIgbWVudWl0 ZW0oaXRlbS0+ZGF0YSgpKTsKICAgICAgICAgICAgIGFwcGVuZChzZWN0aW9uTWVudSwgbWVudWl0 ZW0pOworICAgICAgICAgICAgYnJlYWs7CisgICAgICAgIH0KICAgICAgICAgfQogICAgIH0KICAg ICBndGtfbWVudV9zaGVsbF9iaW5kX21vZGVsKEdUS19NRU5VX1NIRUxMKG1fbWVudSksIEdfTUVO VV9NT0RFTChtZW51LmdldCgpKSwgbnVsbHB0ciwgVFJVRSk7CmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViS2l0L1VJUHJvY2Vzcy9ndGsvV2ViQ29udGV4dE1lbnVQcm94eUd0ay5oIGIvU291cmNlL1dl YktpdC9VSVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuaAppbmRleCAzY2I0MTM1 MmVhNi4uOGIyZTIzODA5ZTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL2d0 ay9XZWJDb250ZXh0TWVudVByb3h5R3RrLmgKKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3Mv Z3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuaApAQCAtNTksNiArNTksNyBAQCBwcml2YXRlOgog ICAgIHZvaWQgYXBwZW5kKEdNZW51KiwgY29uc3QgV2ViQ29udGV4dE1lbnVJdGVtR2xpYiYpOwog ICAgIEdSZWZQdHI8R01lbnU+IGJ1aWxkTWVudShjb25zdCBWZWN0b3I8V2ViQ29udGV4dE1lbnVJ dGVtR2xpYj4mKTsKICAgICB2b2lkIHBvcHVsYXRlKGNvbnN0IFZlY3RvcjxSZWY8V2ViQ29udGV4 dE1lbnVJdGVtPj4mKTsKKyAgICB2b2lkIHBvcHVsYXRlU3ViTWVudShjb25zdCBXZWJDb250ZXh0 TWVudUl0ZW1EYXRhJiwgVmVjdG9yPFdlYkNvbnRleHRNZW51SXRlbUdsaWI+Jik7CiAgICAgc3Rh dGljIHZvaWQgbWVudVBvc2l0aW9uRnVuY3Rpb24oR3RrTWVudSosIGdpbnQqLCBnaW50KiwgZ2Jv b2xlYW4qLCBXZWJDb250ZXh0TWVudVByb3h5R3RrKik7CiAKICAgICBHdGtXaWRnZXQqIG1fd2Vi VmlldzsK