Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

send pageErrors to devtools console #21678

Closed
wants to merge 1 commit into from

Conversation

@AnshulMalik
Copy link
Contributor

AnshulMalik commented Sep 11, 2018

Fixes : #13161

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

Handle pageErrors differently than normal console messages, since these contains stacktrace, which will be supported soon.


This change is Reviewable

@highfive
Copy link

highfive commented Sep 11, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/globalscope.rs, components/script/devtools.rs
  • @KiChjang: components/script/dom/globalscope.rs, components/script/devtools.rs
@highfive
Copy link

highfive commented Sep 11, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm assigned jdm and unassigned paulrouget Oct 31, 2018
Copy link
Member

jdm left a comment

I'm sorry that this was ignored for so long - this looks like a good improvement!

@@ -377,10 +376,34 @@ impl GlobalScope {

// Step 9.
if event_status == EventStatus::NotCanceled {
if let Some(chan) = self.devtools_chan() {

This comment has been minimized.

@jdm

jdm Oct 31, 2018

Member

We should only report the error to the devtools if we don't forward it to a worker object in the code that follows this block.

timeStamp: 0,
error: false,
warning: false,
exception: false,

This comment has been minimized.

@jdm

jdm Oct 31, 2018

Member

This should be true.

@@ -83,6 +83,7 @@ pub enum ScriptToDevtoolsControlMsg {
),
/// A particular page has invoked the console API.
ConsoleAPI(PipelineId, ConsoleMessage, Option<WorkerId>),
PageErrorAPI(PipelineId, PageErrorAPI, Option<WorkerId>),

This comment has been minimized.

@jdm

jdm Oct 31, 2018

Member

Add /// An unhandled exception has occurred in a particular page. Also I think PageError is more clear; the previous message is ConsoleAPI because it is a message for the methods of the Console API.

@@ -285,6 +294,37 @@ fn run_server(
actors.register(Box::new(thread));
}

fn handle_console_page_error_message(

This comment has been minimized.

@jdm

jdm Oct 31, 2018

Member

Let's call this handle_page_error_message.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2018

The latest upstream changes (presumably #22086) made this pull request unmergeable. Please resolve the merge conflicts.

@AnshulMalik
Copy link
Contributor Author

AnshulMalik commented Dec 10, 2018

I'll come back later :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.