Bug 175130

Summary: [GTK][WPE] Add API to provide browser information required by automation
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, bugs-noreply, buildbot, joepeck, mcatanzaro
Priority: P2 Keywords: Gtk
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 174618    
Bug Blocks:    
Attachments:
Description Flags
Patch
bburg: review-
Rebased patch
none
Try to fix WPE build
none
Patch for landing none

Description Carlos Garcia Campos 2017-08-03 06:10:30 PDT
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.
Comment 1 Carlos Garcia Campos 2017-08-03 06:22:48 PDT
Created attachment 317116 [details]
Patch
Comment 2 Carlos Garcia Campos 2017-08-03 06:25:34 PDT
It doesn't apply because it depends on bug #174618
Comment 3 BJ Burg 2017-08-03 17:22:59 PDT
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
Comment 4 BJ Burg 2017-08-04 10:53:18 PDT
Comment on attachment 317116 [details]
Patch

r- until rebased for EWS.
Comment 5 Michael Catanzaro 2017-08-04 12:20:53 PDT
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.
Comment 6 Carlos Garcia Campos 2017-08-05 00:38:52 PDT
(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.
Comment 7 Carlos Garcia Campos 2017-08-05 03:38:32 PDT
(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
Comment 8 Carlos Garcia Campos 2017-08-05 03:40:28 PDT
Created attachment 317335 [details]
Rebased patch
Comment 9 Build Bot 2017-08-05 03:43:27 PDT
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.
Comment 10 Carlos Garcia Campos 2017-08-05 04:20:25 PDT
Created attachment 317336 [details]
Try to fix WPE build
Comment 11 Build Bot 2017-08-05 04:23:27 PDT
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.
Comment 12 Michael Catanzaro 2017-08-05 12:29:27 PDT
GTK API r=me
Comment 13 Carlos Garcia Campos 2017-08-06 01:06:53 PDT
Created attachment 317359 [details]
Patch for landing
Comment 14 Build Bot 2017-08-06 01:08:16 PDT
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.
Comment 15 Carlos Garcia Campos 2017-08-06 23:06:45 PDT
Committed r220329: <http://trac.webkit.org/changeset/220329>