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

fixes #5232, more console log levels sent to devtools #6193

Merged
merged 1 commit into from Jun 2, 2015

Conversation

@j3parker
Copy link
Contributor

j3parker commented May 27, 2015

fixes #5232

The correct styling shows up in the Firefox devtools (e.g. a caution symbol beside warning messages.)

I couldn't quickly find the corresponding Firefox code that handles log-levels so the values I'm sending are "guesses" (but they work seem to work.) I'll look today because I'm sending "log" for Debug-level, Error for failed asserts etc.

Review on Reviewable

@highfive
Copy link

highfive commented May 27, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 27, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5107

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented May 27, 2015

@j3parker
Copy link
Contributor Author

j3parker commented May 27, 2015

@jdm thanks! I'll switch to sending (console.table... interesting.)

Is there a more idiomatic way to serialize the LogLevel enum than the match I'm doing?

@jdm
Copy link
Member

jdm commented May 27, 2015

Not that I can think of.

@nox nox removed the S-awaiting-review label Jun 1, 2015
@nox
Copy link
Member

nox commented Jun 1, 2015

Looks ok to me, did you want to add something more or did something needs to be fixed, @j3parker? You can always implement the rest as a follow-up.

-S-awaiting-review +S-needs-squash


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/devtools/lib.rs @ r3
  • components/devtools_traits/lib.rs @ r1
  • components/script/dom/console.rs @ r1

Comments from the review on Reviewable.io

@nox nox added the S-needs-squash label Jun 1, 2015
@nox nox self-assigned this Jun 1, 2015
@jdm
Copy link
Member

jdm commented Jun 1, 2015

Yep, a+ work. Thanks!

@j3parker
Copy link
Contributor Author

j3parker commented Jun 1, 2015

I'm done as far as I know :) I squashed, thanks.

@jdm
Copy link
Member

jdm commented Jun 1, 2015

@bors-servo: r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 1, 2015

📌 Commit 78125db has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 1, 2015

Testing commit 78125db with merge 534b720...

bors-servo pushed a commit that referenced this pull request Jun 1, 2015
fixes #5232

The correct styling shows up in the Firefox devtools (e.g. a caution symbol beside warning messages.)

I couldn't quickly find the corresponding Firefox code that handles log-levels so the values I'm sending are "guesses" (but they work seem to work.) I'll look today because I'm sending "log" for Debug-level, Error for failed asserts etc.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6193)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 1, 2015

💔 Test failed - linux2

@j3parker
Copy link
Contributor Author

j3parker commented Jun 1, 2015

Urg, I think I messed up while rebasing. Give me a second to see what happened.

@nox
Copy link
Member

nox commented Jun 1, 2015

@j3parker

components/devtools/lib.rs:263:21: 263:37 error: no associated item named `Assert` found for type `devtools_traits::LogLevel` in the current scope
components/devtools/lib.rs:263                     LogLevel::Assert => "error",
                                                   ^~~~~~~~~~~~~~~~
@j3parker
Copy link
Contributor Author

j3parker commented Jun 1, 2015

@nox yup, didn't mean to commit that :P build passes locally now.

@jdm
Copy link
Member

jdm commented Jun 2, 2015

@bors-servo: r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

📌 Commit a00d264 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

Testing commit a00d264 with merge ec79881...

bors-servo pushed a commit that referenced this pull request Jun 2, 2015
bors-servo
fixes #5232

The correct styling shows up in the Firefox devtools (e.g. a caution symbol beside warning messages.)

I couldn't quickly find the corresponding Firefox code that handles log-levels so the values I'm sending are "guesses" (but they work seem to work.) I'll look today because I'm sending "log" for Debug-level, Error for failed asserts etc.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6193)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

💔 Test failed - mac2

@jdm
Copy link
Member

jdm commented Jun 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

Previous build results for android, gonk, linux1, linux2, mac1 are reusable. Rebuilding only mac2...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit a00d264 into servo:master Jun 2, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@j3parker j3parker deleted the j3parker:fix-5232 branch Jun 2, 2015
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.

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