Bug 197271 – [WPE][GTK] Add WebKitWebPage::did-associate-form-controls-for-frame and deprecate original did-associate-form-controls

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

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).

[Michael Catanzaro]

Created attachment 368211 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

Attachment 368211 [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 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Created attachment 368433 [details]
Patch

[EWS Watchlist]

Attachment 368433 [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.

[Michael Catanzaro]

Created attachment 368434 [details]
Patch

[EWS Watchlist]

Attachment 368434 [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.

[Michael Catanzaro]

WK2 owner approval for the cross-platform bits would be great.

[Michael Catanzaro]

Ping for review.

[youenn fablet]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 369281 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 369303 [details]
Patch

[EWS Watchlist]

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.

[Michael Catanzaro]

Comment on attachment 369303 [details]
Patch

Thanks, Youenn.

I need review from Carlos, too.

[Alex Christensen]

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.

[Michael Catanzaro]

As explained above, no check is needed, it's guaranteed to be nonnull. The assert is just there to document that.

[Carlos Garcia Campos]

Comment on attachment 369303 [details]
Patch

LGTM

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

Committed r245174: <https://trac.webkit.org/changeset/245174>