Bug 175130 – [GTK][WPE] Add API to provide browser information required by automation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

When a new automation session is started, the web driver receives some required capabilities from the client, like browser name and version. The session should be rejected if those required capabilities don't match with the actual browser that is launched. We don't know that information in WebKit, so we need to add API so that users can provide it when a new session request is made.

[Carlos Garcia Campos]

Created attachment 317116 [details]
Patch

[Carlos Garcia Campos]

It doesn't apply because it depends on bug #174618

[Blaze Burg]

Comment on attachment 317116 [details]
Patch

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

Source/JavaScriptCore and Source/WebDriver parts look good to me. Please find a GTK reviewer for the rest.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
> +            String browserName;

Perhaps we can conditionalize these for GTK/WPE, I don't think Safari/safaridriver needs them at present and this patch doesn't update the capability caching code for these keys. Each Safari (Safari Technology Preview, system Safari, engineering builds) have their own copy of safaridriver so there is no matching to be done on the browser side.

I think we'll want to have the ability to report and request capabilities for some other things, however.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:93
> +    std::optional<RemoteInspector::Client::Capabilities> clientCapabilities() const { return m_clientCapabilities; }

OK

[Blaze Burg]

Comment on attachment 317116 [details]
Patch

r- until rebased for EWS.

[Michael Catanzaro]

Comment on attachment 317116 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitApplicationInfo.cpp:136
> + * Set the application version. If the application doesn't use the format
> + * major.minor.micro you can pass 0 as the micro to use major.minor, or pass
> + * 0 as both micro and minor to use only major number. Any other format must
> + * be converted to major.minor.micro so that it can be used in version comparisons.

What about nano versions? I do nano versions for Epiphany on occasion.

This API is too limiting. I would make it variadic so the user can add any desired number of decimal places.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 317116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317116&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitApplicationInfo.cpp:136
> > + * Set the application version. If the application doesn't use the format
> > + * major.minor.micro you can pass 0 as the micro to use major.minor, or pass
> > + * 0 as both micro and minor to use only major number. Any other format must
> > + * be converted to major.minor.micro so that it can be used in version comparisons.
> 
> What about nano versions? I do nano versions for Epiphany on occasion.
> 
> This API is too limiting. I would make it variadic so the user can add any
> desired number of decimal places.

I don't think we need that level of detail. This is used for drivers to request a  minimum version of the browser, normally because it has a new feature required or something like that, I doubt we really need to require a nano version ever. Also, this should work for projects using any other version formats, they are expected to convert them into x.y.z ensuring they can be compared. So, the selenium driver is always going to use x.y.x format.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #3)
> Comment on attachment 317116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317116&action=review
> 
> Source/JavaScriptCore and Source/WebDriver parts look good to me. Please
> find a GTK reviewer for the rest.
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
> > +            String browserName;
> 
> Perhaps we can conditionalize these for GTK/WPE, I don't think
> Safari/safaridriver needs them at present and this patch doesn't update the
> capability caching code for these keys. Each Safari (Safari Technology
> Preview, system Safari, engineering builds) have their own copy of
> safaridriver so there is no matching to be done on the browser side.

My idea was to make it glib only initially, but I think this doesn't hurt when unused and we leave cleaner code without ifdefs.

> I think we'll want to have the ability to report and request capabilities
> for some other things, however.
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:93
> > +    std::optional<RemoteInspector::Client::Capabilities> clientCapabilities() const { return m_clientCapabilities; }
> 
> OK

[Carlos Garcia Campos]

Created attachment 317335 [details]
Rebased patch

[Build Bot]

Attachment 317335 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitAutomationSession.h"
ERROR: Source/WebDriver/SessionHost.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/SessionHost.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitApplicationInfo.h"
ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:245:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitApplicationInfo.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/webkit.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitAutomationSession.h"
Total errors found: 3 in 27 files


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

[Carlos Garcia Campos]

Created attachment 317336 [details]
Try to fix WPE build

[Build Bot]

Attachment 317336 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitAutomationSession.h"
ERROR: Source/WebDriver/SessionHost.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/SessionHost.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitApplicationInfo.h"
ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:245:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitApplicationInfo.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/webkit.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitAutomationSession.h"
Total errors found: 3 in 27 files


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

[Michael Catanzaro]

GTK API r=me

[Carlos Garcia Campos]

Created attachment 317359 [details]
Patch for landing

[Build Bot]

Attachment 317359 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitAutomationSession.h"
ERROR: Source/WebDriver/SessionHost.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/SessionHost.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitApplicationInfo.h"
ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:245:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitApplicationInfo.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/webkit.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitAutomationSession.h"
Total errors found: 3 in 27 files


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

[Carlos Garcia Campos]

Committed r220329: <http://trac.webkit.org/changeset/220329>