Update tooling and protocols by svatal · Pull Request #19 · MasterDevs/ChromeDevTools

svatal · 2017-08-12T09:30:58Z

merged abandoned georgiosd's PR Update tooling and protocols #15
some other updates (protocols from Chrome 60, headless mode, example takes screenshot of the page, SendAsync returns correct response)

… of having to instantiate it

protocol conflicts resolved using master - regeneration needed

…the page)

.. so user does not have to use cast on every command result that returns something interesting

brewdente · 2017-10-24T19:19:32Z

source/ChromeDevTools/ChromeProcessFactory.cs

-            var chromeProcessArgs = remoteDebuggingArg + " " + userDirectoryArg + " --bwsi --no-first-run";
+            var remoteDebuggingArg = $"--remote-debugging-port={port}";
+            var userDirectoryArg = $"--user-data-dir=\"{directoryInfo.FullName}\"";
+            const string headlessArg = "--headless --disable-gpu";


Are we forcing headless here? I'm not comfortable doing this, unless others feel strongly?

I'll add an option.

brewdente · 2017-10-24T19:26:20Z

source/ChromeDevTools/IChromeSession.cs

    public interface IChromeSession
    {
-        Task<ICommandResponse> SendAsync<T>(T parameter, CancellationToken cancellationToken);
+        Task<CommandResponse<TResponse>> SendAsync<TResponse>(ICommand<TResponse> parameter, CancellationToken cancellationToken);


Is this correct? There was a reason it was just returning the interface. I'm having a hard time recalling the reason though. @qmfrederik do you have thoughts on this?

Hmm, now I see there is second implementation of ICommandResponse - ErrorResponse. And it can be returned from the task ..
I'd personally prefer setting task to faulty in such cases than to be forced to check and retype every command. What are your thoughts?

I ran through the history via git blame but I don't think I changed this to return an interface, it looks like it just always has been like that. (I did submit a bunch of other PRs to return interfaces in other areas, but not here).

I think this change is OK, since CommandResponse<T> is a very simple class.

Ok, i really like this change, i'm just afraid of "breaking" other people using this.

brewdente · 2017-10-24T20:14:02Z

source/Sample/Program.cs

+                            await chromeSession.SendAsync(new SetVisibleSizeCommand {Width = ViewPortWidth, Height = height});
+
+                            Console.WriteLine("Taking screenshot");
+                            var screenshot = await chromeSession.SendAsync(new CaptureScreenshotCommand {Format = "png"});


I do like this change ... it does seem like more advanced example. Would you be opposed to making a Simple and Advanced sample? Then we can link them from the README.

Do you mean keeping the original sample too?

Yeah ... maybe we can even make it simpler

Something for later though

qmfrederik · 2017-10-25T09:29:29Z

@svatal Do you think it would be possible to split off the protocol update in a separate PR? That would make the PR easier to review 😄 .

svatal · 2017-10-25T10:44:10Z

@qmfrederik Hmm, I do not see a easy way how to do it - both code and protocol changes depends on each other. I had to fix protocol generator after updating protocol and on the same time the newly generated code is needed to return correct result from SendAsync

qmfrederik · 2017-10-25T17:31:08Z

@brewdente Cool to see this got merged. Any chance we get an updated NuGet package, too? 😄

svatal · 2017-10-26T08:24:20Z

@brewdente Really happy to see the changes merged, but what about the ErrorResponse?
Before, it has been an user's obligation to check the response if no error was returned and just then cast it to correct response type (although I doubt people were doing it).
Now, the error case is unhandled and it will throw while casting and the ErrorResponse will be lost.
What can be done from my point of view:
a) throw intentionally with the ErrorResponse included
b) return a proxy-response that will have isError, Result and Error and throw nice error when accessing the wrong getter.
I was initially rooting for the a) case, but while polishing the b) case, it seems to be perfectly suitable to both usages (either checking for error first or going the happy path), so I'll try to implement the second option.

svatal · 2017-10-26T12:59:18Z

should be solved in PR #23

brewdente · 2017-10-26T15:16:48Z

@qmfrederik heh ... i was doing it anyways. but i'm glad you were a fan of it too. I questioned the nuget version number ... ultimately went with 1.1.0 (which I think is right semantically).

georgiosd and others added 20 commits March 15, 2017 21:24

add recent protocols

0b33a4c

add missing members from domain objects

79c6319

support multiple json files

88e737d

update protocol

838450f

fix tests

7a9d957

allow empty classes to be generated

c61610a

split process object to allow connecting to a remote instance instead…

d29593e

… of having to instantiate it

allow starting new sessions

f00120d

fix example

b90b76c

fix endpoint for create

f37da53

Merge branch 'master' of https://github.com/georgiosd/ChromeDevTools

2414749

protocol conflicts resolved using master - regeneration needed

regenerated protocol

0ee8ca6

command test fixed to use new protocol file

548620f

new protocols - Chrome 60

8203b74

fixed generator not to prefix native types

0e1d4a7

run chrome in headless mode & wait for user dir deletion

4f74d04

added example that is actually doing something (taking screenshot of …

973567a

…the page)

SendAsync returns correct result type

fd8aa43

.. so user does not have to use cast on every command result that returns something interesting

trying to satisfy dotnet core compiler

e58cf65

cleanup

81f12e8

brewdente reviewed Oct 24, 2017

View reviewed changes

chrome - ability to set headless mode and path to chrome

700d409

brewdente merged commit c4bdc6d into MasterDevs:master Oct 25, 2017

Conversation

svatal commented Aug 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qmfrederik commented Oct 25, 2017

Uh oh!

svatal commented Oct 25, 2017

Uh oh!

qmfrederik commented Oct 25, 2017

Uh oh!

svatal commented Oct 26, 2017

Uh oh!

svatal commented Oct 26, 2017

Uh oh!

brewdente commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants