Skip to content

Comments

[UWP] Basic console panel#26952

Merged
bors-servo merged 2 commits intoservo:masterfrom
paulrouget:console
Jun 18, 2020
Merged

[UWP] Basic console panel#26952
bors-servo merged 2 commits intoservo:masterfrom
paulrouget:console

Conversation

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 17, 2020

This PR introduces a Devtools client module to the UWP app. And a basic console panel.

There are still bugs and missing features, that I will address in future PRs:

  • the output is just some text. No colors or formatting. This will be implemented as a gridview later.
  • When connecting to the devtools server, we get a prompt asking for permission. I see 2 options: we share the socket between Servo and the UWP app (no idea if that's actually possible) or we keep using the TCP transport and share a token between Servo and the UWP app that would allow to bypass the permission prompt
  • add a JS input.
  • show HTTP errors. For now they are just ignored.
  • There are 3 panels now: console, preference and a random "devtool server" panel that shows the devtools port. We could just print the port number in the console instead.
  • Servo crashes if we open the devtools too early. This needs investigation.

Also, now that we have the foundation of a devtools client in UWP, we should remove the recently introduced preference API and rely on the preference devtools actor instead.

Fix #26850

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 17, 2020
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greta work! I was worried the JSON code in C++ would be complex, but the client code is actually surprisingly readable!

@jdm
Copy link
Member

jdm commented Jun 17, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit e4041e0 has been approved by jdm

@highfive highfive assigned jdm and unassigned asajeffrey Jun 17, 2020
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 17, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit e4041e0 with merge 158afb2...

bors-servo added a commit that referenced this pull request Jun 17, 2020
[UWP] Basic console panel

This PR introduces a Devtools client module to the UWP app. And a basic console panel.

There are still bugs and missing features, that I will address in future PRs:

- the output is just some text. No colors or formatting. This will be implemented as a [gridview](https://docs.microsoft.com/en-us/dotnet/framework/wpf/controls/gridview-overview) later.
- When connecting to the devtools server, we get a prompt asking for permission. I see 2 options: we share the socket between Servo and the UWP app (no idea if that's actually possible) or we keep using the TCP transport and share a token between Servo and the UWP app that would allow to bypass the permission prompt
- add a JS input.
- show HTTP errors. For now they are just ignored.
- There are 3 panels now: console, preference and a random "devtool server" panel that shows the devtools port. We could just print the port number in the console instead.
- Servo crashes if we open the devtools too early. This needs investigation.

Also, now that we have the foundation of a devtools client in UWP, we should remove the recently introduced preference API and rely on the preference devtools actor instead.

Fix #26850
@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 17, 2020
@jdm
Copy link
Member

jdm commented Jun 18, 2020

support/hololens/ServoApp/Devtools/Client.cpp is not formatted correctly.
Run `./mach fmt` to fix the formatting

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 18, 2020
@jdm
Copy link
Member

jdm commented Jun 18, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit bd8c7d6 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 18, 2020
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 18, 2020
@jdm
Copy link
Member

jdm commented Jun 18, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit c7f5d8a has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 18, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit c7f5d8a with merge 57e7516...

@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 57e7516 to master...

@bors-servo bors-servo merged commit 57e7516 into servo:master Jun 18, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 18, 2020
@bors-servo bors-servo mentioned this pull request Jun 18, 2020
5 tasks
@jdm
Copy link
Member

jdm commented Jun 18, 2020

@RaananW As of today's nightly, you can now pop open a panel in Firefox Reality that shows the console messages and JS exceptions for the current page. It's the default view when clicking the wrench button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UWP: Implement a console panel (devtools)

5 participants