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

devtools: Implemented `console.group` and `console.groupEnd` #16251

Closed
wants to merge 3 commits into from

Conversation

@HarryLovesCode
Copy link
Contributor

HarryLovesCode commented Apr 3, 2017

  • Modified WebIDL file to allow for Group, GroupCollapsed, GroupEnd functions
  • Added log level Group, GroupCollapsed, and GroupEnd
  • Added functionality for preparing and sending messages with groups

Notes

  • Needs logging. This commit (currently) does not complete issue #9274
  • Needs console.groupCollapsed implementation

Nightly Edition without Group Labels

Nightly edition

Developer Edition without Group Labels

Developer edition

Nightly Edition with Group Labels

Nightly with group labels


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because there are no means to test the developer tools currently

This change is Reviewable

Made having a group label optional and followed Firefox implementation
@highfive
Copy link

highfive commented Apr 3, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/webidls/Console.webidl, components/script/dom/console.rs, components/devtools/lib.rs, components/devtools_traits/lib.rs, components/devtools_traits/lib.rs
  • @KiChjang: components/script/dom/webidls/Console.webidl, components/script/dom/console.rs
@highfive
Copy link

highfive commented Apr 3, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Apr 5, 2017

Why'd you revert the commit?

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Apr 5, 2017

@jdm GitKraken glitch sorry about that..

Copy link
Member

jdm left a comment

One change should avoid a bunch of the redundant DOMString::new() additions.

}

fn prepare_message(log_level: LogLevel, message: DOMString) -> ConsoleMessage {
fn prepare_message(log_level: LogLevel, message: DOMString, group_name: DOMString) -> ConsoleMessage {

This comment has been minimized.

Copy link
@jdm

jdm Apr 5, 2017

Member

Given that only Group passes a real value for this, and it's a copy of the message, why not remove the extra argument and infer the groupName value based on the log level?

This comment has been minimized.

Copy link
@HarryLovesCode

HarryLovesCode Apr 6, 2017

Author Contributor

Are you suggesting removing the argument from prepare_message and, since we are sending the label as the message, using that instead of the extra argument?

This comment has been minimized.

Copy link
@jdm

jdm Apr 7, 2017

Member

That is correct.

@jdm jdm assigned jdm and unassigned glennw Apr 5, 2017
@jdm
Copy link
Member

jdm commented Apr 5, 2017

As for tests, we do not have any automated way of testing our devtools integration, unfortunately. I'm not sure what "needs logging" means.

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Apr 7, 2017

@jdm Side question, any reason Servo's WebIDL's are statically typed?

// Servo
void log(DOMString... messages);
void info(DOMString... messages);
// Firefox
void log(any... data);
void info(any... data);
@jdm jdm added S-needs-squash and removed S-awaiting-review labels Apr 7, 2017
@jdm
Copy link
Member

jdm commented Apr 7, 2017

Probably because that was the easiest way to get some kind of usable string value that we could print to the terminal. Firefox does more complicated formatting than toString(), so it makes sense to take an actual object.

Why doesn't this PR complete #9274? It looks like it's ready to merge if you squash the two commits into one.

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Apr 7, 2017

@jdm Isn't #9274 referring to logging to the command line?

@jdm
Copy link
Member

jdm commented Apr 7, 2017

Ooh, good point.

@jdm jdm removed the S-awaiting-review label Apr 7, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2017

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

@jdm
Copy link
Member

jdm commented Nov 15, 2017

Closing due to inactivity.

@jdm jdm closed this Nov 15, 2017
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.