M1452: Initial Step of Integrate an XML parser #3718
Conversation
|
Critic review: https://critic.hoppipolla.co.uk/r/2901 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 |
|
Thanks @nchinth! This is a good start, but I think it makes sense to hold off on merging these changes until the Parser trait is implemented by the HTML parser. |
|
Additionally, I think the Parser trait belongs in components/script/parse/mod.rs, which you can create. Then the trait can be imported as parse::Parser in the future. |
|
@nchinth This is better, but not equivalent - the changes in the commits cause every call to feed to be followed by end, when end really needs to happen when all parsing is finished. The Parser trait should probably grow another method to encapsulate that. |
|
Sure Josh, shall i add another method like release_resources to Parser trait and call the end method in its implementation for ServoHTMLParser? |
|
Let's just call it finish. |
|
Yep Sure. |
|
Almost there! The new file needs a license header, but otherwise this looks really good! |
|
I've added a few more comments to the Critic review. Please make those changes, commit them, and then rebase your changes on top master following the instructions at https://github.com/servo/servo/wiki/Github-&-Critic-PR-handling-101 ! |
nchinth
force-pushed
the
master
branch
2 times, most recently
from
33ba093
to
9931ff2
Compare
|
One very small issue left and this will be ready! |
|
Thanks Josh. Fixed it as well. |
|
Great! Please squash all these commits together and I'll merge them! |
|
fwiw this will move away from |
nchinth
changed the title
nchinth
changed the title
nchinth
changed the title
|
Hi Josh. I have squashed all of them into one one commit |
|
Thanks @nchinth! |
We have created parser trait and declared parse_chunk function in this trait. We are yet to implement this parse_chunk for ServoHTMLParser struct.
|
Hi Josh, The failure is from "test-tidy" shell script and looks like its because of tabular spaces . The error message is as follows Should i remove the tabs or would it be fine like this ? |
|
No, it's not fine like this, that's why we have automatic tests for it. Remove the tabs. |
Added parse_chunk method declaration to parser Removed unnecessary visibilty for parse_chunk function Implemented parse_chunk function Implemented parse_chunk fn for ServoHTMLParser Moved parser trait to mod.rs and added finish fn added licence header to mod.rs and other review comments Fixed trailing space issue Fixed failed tabular space test
|
I have fixed this. Thanks. |
We have created parser trait and declared parse_chunk function in this trait. We are yet to implement this parse_chunk for ServoHTMLParser struct.
We have created parser trait and declared parse_chunk function in this trait. We are yet to implement this parse_chunk for ServoHTMLParser struct.