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

rename #11480

Merged
merged 1 commit into from Jun 1, 2016
Merged

rename #11480

merged 1 commit into from Jun 1, 2016

Conversation

@rafaqtro
Copy link
Contributor

rafaqtro commented May 27, 2016

rename of page_fetch_complete and handle_page_fetch_complete for page_headers_available and handle_page_headers_available .


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11418 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented May 27, 2016

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

@highfive
Copy link

highfive commented May 27, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/script_thread.rs
@highfive
Copy link

highfive commented May 27, 2016

warning Warning warning

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

jdm commented May 27, 2016

@rafaqtro It doesn't appear that you have compiled these changes, since no code that calls page_fetch_complete was updated.

@highfive
Copy link

highfive commented May 28, 2016

New code was committed to pull request.

@emilio
Copy link
Member

emilio commented May 28, 2016

Hi, this looks a lot better now! :)

Could you align the arguments for those functions to the same column, please?
This is ready to land with that.

Thanks for doing this! :-)

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/script_thread.rs, line 481 [r1] (raw file):

impl ScriptThread {
    pub fn page_headers_available(id: &PipelineId, subpage: Option<&SubpageId>, metadata: Option<Metadata>)
                               -> Option<ParserRoot> {

nit: Could you align this with the parenthesis, as it was before?


components/script/script_thread.rs, line 1302 [r1] (raw file):

    /// Kick off the document and frame tree creation process using the result.
    fn handle_page_headers_available(&self, id: &PipelineId, subpage: Option<&SubpageId>,
                                  metadata: Option<Metadata>) -> Option<ParserRoot> {

ditto


components/script/dom/servohtmlparser.rs, line 98 [r2] (raw file):

        let content_type = metadata.clone().and_then(|meta| meta.content_type);
        let parser = match ScriptThread::page_headers_available(&self.id,
                                                             self.subpage.as_ref(),

here too, please :)


Comments from Reviewable

@emilio
Copy link
Member

emilio commented May 28, 2016

Also, if you could squash the commits, that would be great, let us know if you need any help with that :)

@rafaqtro
Copy link
Contributor Author

rafaqtro commented May 28, 2016

yes, @emilio how do i squash the commits?

@jdm
Copy link
Member

jdm commented May 29, 2016

@highfive
Copy link

highfive commented May 31, 2016

New code was committed to pull request.

@KiChjang
Copy link
Member

KiChjang commented May 31, 2016

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2016

📌 Commit 71104c8 has been approved by emilio

@emilio
Copy link
Member

emilio commented May 31, 2016

Great, thanks for doing this @rafaqtro! :)

@bors-servo: r+

Take a look to https://starters.servo.org if you want to find another issue, we're happy to help!

@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #11522
@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2016

📌 Commit 71104c8 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jun 1, 2016

Testing commit 71104c8 with merge 5d86b9b...

bors-servo added a commit that referenced this pull request Jun 1, 2016
rename

<!-- Please describe your changes on the following line: -->
rename of page_fetch_complete and handle_page_fetch_complete for page_headers_available and handle_page_headers_available .

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11418 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11480)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 1, 2016

@bors-servo bors-servo merged commit 71104c8 into servo:master Jun 1, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@rafaqtro rafaqtro deleted the rafaqtro:local_b branch Jun 3, 2016
@rafaqtro
Copy link
Contributor Author

rafaqtro commented Jun 3, 2016

yes i would like to find other issue. if you can help me @emilio ?

@emilio
Copy link
Member

emilio commented Jun 3, 2016

Hi @rafaqtro,

So, would you like something like #11557, or maybe something a bit more challenging? If you'd prefer to use other languages, like JavaScript or Rust, there are also good first PRs for those, what would you be more interested in?

@rafaqtro
Copy link
Contributor Author

rafaqtro commented Jun 3, 2016

yes, i do this, and after that, i could do some more challenging maybe in js.

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.

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