Added basic console.table() implementation #29689
Conversation
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
switchpiggy
force-pushed
the
switchpiggy
branch
from
9c4c279
to
92b34c3
Compare
switchpiggy
force-pushed
the
switchpiggy
branch
from
92b34c3
to
ef35292
Compare
components/script/dom/console.rs
Outdated
| tabular_data: HandleValue, | ||
| properties: Option<Vec<DOMString>>, | ||
| ) { | ||
| unsafe { |
There was a problem hiding this comment.
This unsafe block is very wide. Try to move it down to exactly the problematic code and then let's see if we can eliminate it entirely.
There was a problem hiding this comment.
The problematic part is calling the generate_table() function, which is an unsafe function because it calls Vec::from_jsval() and is_array_like(). I'm not very experienced with the servo documentation, so there could be a way to implement generate_table() safely which I'm not aware of.
There was a problem hiding this comment.
At the very least here, I think you should reduce the unsafe block to enclose only the unsafe operations instead of marking the entire function as unsafe.
There was a problem hiding this comment.
Okay, sure! I've removed the unsafe block around generate_table() and just restricted the unsafe blocks to the unsafe operations inside the function.
components/script/dom/console.rs
Outdated
| for row in table.iter() { | ||
| for col in row.iter() { | ||
| res.push_str(col as &str); | ||
| res.push_str("\t\t"); |
There was a problem hiding this comment.
If we implement this, it deserves an implementation that knows how to properly align the columns of the table. Two tabs are pretty brittle, so maybe investigate if there is a more resilient way to do this based on the actual strings that we are processing.
There was a problem hiding this comment.
That's a good idea. I'll see if there's a good way to align the columns.
There was a problem hiding this comment.
I've implemented a simple way to align the columns by setting each column's width to be the maximum length of all of its rows, plus some extra space.
…ndling that supports inputting primitive arrays.
…rror handling that supports inputting primitive arrays.
components/script/dom/console.rs
Outdated
| @@ -65,13 +73,76 @@ fn console_message(global: &GlobalScope, message: DOMString, level: LogLevel) { | |||
| }) | |||
| } | |||
|
|
|||
| ///Generates matrix of table elements, and then converts matrix into a properly formatted DOMString | |||
There was a problem hiding this comment.
When making a comment in rust you should put a space after the slashes. In this case, I don't think the rustdoc will work without the space.
There was a problem hiding this comment.
Oh ok sure! Thanks for the suggestion, I'm not really familiar with rustdoc so that really helps. I'll make sure to fix the comment.
Implemented a basic version of console.table() that supports arrays of primitives as input. The full functionality as described in the spec as mentioned in #27212 will also be implemented later.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors