Bug 197271 - [WPE][GTK] Add WebKitWebPage::did-associate-form-controls-for-frame and deprecate original did-associate-form-controls
Summary: [WPE][GTK] Add WebKitWebPage::did-associate-form-controls-for-frame and depre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-24 21:34 PDT by Michael Catanzaro
Modified: 2019-05-10 09:05 PDT (History)
13 users (show)

See Also:


Attachments
Patch (19.15 KB, patch)
2019-04-24 21:38 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (20.24 KB, patch)
2019-04-28 09:22 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (20.25 KB, patch)
2019-04-28 11:44 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (20.48 KB, patch)
2019-05-07 07:26 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (20.63 KB, patch)
2019-05-07 11:14 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-04-24 21:34:11 PDT
WebKitWebPage::did-associate-form-controls should provide a WebKitFrame argument so that Epiphany can run JS in the frame the form exists in rather than only the main frame. But we can't do that without breaking API, so add a new signal instead and deprecate the old one.

Note this needs a WK2 owner.
Comment 1 Michael Catanzaro 2019-04-24 21:37:26 PDT
Note the point of deprecating is to remind us to get rid of did-associate-form-controls-for-frame and add the parameter to did-associate-form-controls next time we break API (e.g. for GTK 4).
Comment 2 Michael Catanzaro 2019-04-24 21:38:07 PDT
Created attachment 368211 [details]
Patch
Comment 3 EWS Watchlist 2019-04-24 21:40:48 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 EWS Watchlist 2019-04-24 21:41:04 PDT Comment hidden (obsolete)
Comment 5 Michael Catanzaro 2019-04-28 09:22:33 PDT
Created attachment 368433 [details]
Patch
Comment 6 EWS Watchlist 2019-04-28 09:24:57 PDT Comment hidden (obsolete)
Comment 7 Michael Catanzaro 2019-04-28 11:44:54 PDT
Created attachment 368434 [details]
Patch
Comment 8 EWS Watchlist 2019-04-28 11:47:15 PDT Comment hidden (obsolete)
Comment 9 Michael Catanzaro 2019-04-28 13:05:10 PDT
WK2 owner approval for the cross-platform bits would be great.
Comment 10 Michael Catanzaro 2019-05-06 18:59:06 PDT
Ping for review.
Comment 11 youenn fablet 2019-05-06 19:11:47 PDT
Comment on attachment 368434 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7007
> +        ASSERT(m_frame);

What prevents m_frame from being null here?

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:175
> +    m_client.didAssociateFormControlsForFrame(toAPI(page), toAPI(API::Array::create(WTFMove(elementHandles)).ptr()), toAPI(frame), m_client.base.clientInfo);

If didAssociateFormControlsForFrame is implemented, we probably do not want to call didAssociateFormControls.
Comment 12 Michael Catanzaro 2019-05-07 07:19:19 PDT
Comment on attachment 368434 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7007
>> +        ASSERT(m_frame);
> 
> What prevents m_frame from being null here?

It's guaranteed by:

Page* Document::page() const
{
    return m_frame ? m_frame->page() : nullptr;
}

so I preferred to write it out as an ASSERT, rather than add an unneeded conditional or leave it ambiguous whether a conditional was missed here.

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:175
>> +    m_client.didAssociateFormControlsForFrame(toAPI(page), toAPI(API::Array::create(WTFMove(elementHandles)).ptr()), toAPI(frame), m_client.base.clientInfo);
> 
> If didAssociateFormControlsForFrame is implemented, we probably do not want to call didAssociateFormControls.

I'm not sure what advantage that would have, but either way seems reasonable to me, so OK.
Comment 13 Michael Catanzaro 2019-05-07 07:26:43 PDT
Created attachment 369281 [details]
Patch
Comment 14 EWS Watchlist 2019-05-07 07:28:25 PDT
Attachment 369281 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:429:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 youenn fablet 2019-05-07 08:27:52 PDT
Comment on attachment 369281 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:163
>          return;

This check should probably be updated so that a client that sets didAssociateFormControlsForFrame does not have to set didAssociateFormControls.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:172
> +        m_client.didAssociateFormControls(toAPI(page), toAPI(API::Array::create(WTFMove(elementHandles)).ptr()), m_client.base.clientInfo);

The idea is that the clients should transition from the old one to the new one.
Once a client opt in the new callback, it seems better for the old callback to become a no-op.
Comment 16 Michael Catanzaro 2019-05-07 11:14:04 PDT
(In reply to youenn fablet from comment #15)
> Comment on attachment 369281 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369281&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageFormClient.cpp:163
> >          return;
> 
> This check should probably be updated so that a client that sets
> didAssociateFormControlsForFrame does not have to set
> didAssociateFormControls.

Oops.
Comment 17 Michael Catanzaro 2019-05-07 11:14:39 PDT
Created attachment 369303 [details]
Patch
Comment 18 EWS Watchlist 2019-05-07 11:19:28 PDT
Attachment 369303 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:429:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Michael Catanzaro 2019-05-07 12:51:38 PDT
Comment on attachment 369303 [details]
Patch

Thanks, Youenn.

I need review from Carlos, too.
Comment 20 Alex Christensen 2019-05-07 13:13:47 PDT
Comment on attachment 369303 [details]
Patch

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

LGTM too

> Source/WebCore/dom/Document.cpp:6963
> +        ASSERT(m_frame);

I think we should null check this.  Otherwise I think we'll introduce a rare crash.
Comment 21 Michael Catanzaro 2019-05-07 13:15:44 PDT
As explained above, no check is needed, it's guaranteed to be nonnull. The assert is just there to document that.
Comment 22 Carlos Garcia Campos 2019-05-10 02:31:33 PDT
Comment on attachment 369303 [details]
Patch

LGTM
Comment 23 Carlos Garcia Campos 2019-05-10 02:33:27 PDT
Comment on attachment 369303 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:571
> +     * Deprecated: 2.26, use WebKitWebPage::form-controls-associated-for-frame instead.

I think you need to use #WebKitWebPage::form-controls-associated-for-frame for the link to be created.
Comment 24 Michael Catanzaro 2019-05-10 09:05:39 PDT
Committed r245174: <https://trac.webkit.org/changeset/245174>