Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Add emulation commands to chrome connection #68

Merged

Conversation

PatoBeltran
Copy link
Contributor

Add emulation chrome commands which are helpful for viewport resizing.

@msftclas
Copy link

Hi @PatoBeltran, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Patricio Saldivar). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@auchenberg
Copy link
Contributor

The purpose of the core debugger is not to provide a client library for the APIs supported for by Chrome and other Chrome Debugger Protocol compliant runtimes.

If this is needed I suggest that we could add a generic method to trigger commands on the connected CDP connection, @roblourens?

@nojvek
Copy link
Contributor

nojvek commented Jul 13, 2016

Who will be calling the functions? Aren't they going to be dead functions for time being?

@roblourens
Copy link
Member

I told Patricio to add these, to hold vscode-cordova over until we do something like #46

@roblourens roblourens merged commit 6401d7d into microsoft:master Jul 13, 2016
@auchenberg
Copy link
Contributor

auchenberg commented Jul 13, 2016

I see this as a slippery slope. These should be added in the runtime specific debugger, and not in the core. The core should expose generic logic to make API requests. You just created hard dependency to a Chrome specific API that isn't related to script debugging.

@roblourens
Copy link
Member

ChromeConnection is sort of the generic logic to make API requests. It will be outsourced to some external module, which will be pretty easy because it's already totally separate from the actual logic in -core.

@auchenberg
Copy link
Contributor

And yet you just expanded the API surface exposed by core to include API Emulation features that will you will need to support in the future to be backwards compatible?

I made my point, but you are the maintainer, so it's your call.

@roblourens
Copy link
Member

Well, I'm not concerned about backwards compatibility here, to be honest. This interface will break when we do #46 and replace ChromeConnection with some other library that provides the CDP endpoint. And that interface should break whenever the protocol changes.

@nojvek
Copy link
Contributor

nojvek commented Jul 14, 2016

That's right, the goal of crdi is to pull nightly changes from chromium
protocol jsons, generate an interface like this and publish to npm if any
changes.

Since it has both the adapter and client interfaces, the edge adapter can
implement the adapter interface and vscode / other clients can use the
client interface.

The request, response, event object interfaces are shared between client
and server.

e.g.

https://github.com/nojvek/chrome-remote-debug-interface/blob/master/src/crdi.ts

On Wed, Jul 13, 2016 at 5:20 PM, Rob Lourens notifications@github.com
wrote:

Well, I'm not concerned about backwards compatibility here, to be honest.
This interface will break when we do #46
#46 and
replace ChromeConnection with some other library that provides the CDP
endpoint. And that interface should break whenever the protocol changes.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#68 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA-JVP-EosOWBxbYkGXocX_0SCXZzW-gks5qVYDWgaJpZM4JL84W
.

@auchenberg
Copy link
Contributor

Let me keep you in mind that the supported API is an dynamic target and depend on the target client, hence the added ability to fetch protocol.json on demand on other clients and in runtimes like Edge and Node.

Relaying on the Chromium specific API would be a short term thing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants