Skip to content

/ servo

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

Remove FrameChange::document_ready #14724

Merged
merged 2 commits into from Dec 25, 2016
Merged

Remove FrameChange::document_ready #14724

merged 2 commits into from Dec 25, 2016

Conversation

@zaynetro
Copy link

zaynetro commented Dec 25, 2016 •
edited by larsbergstrom

Remove unused FrameChange::document_ready field and remove unused boolean in ConstellationMsg::GetPipeline's second argument.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14693
  • There are tests for these changes OR
  • These changes do not require tests because compiler validates the field is not used anywhere

This change is Reviewable
Roman Zaynetdinov added 2 commits Dec 25, 2016
Roman Zaynetdinov
Remove unused FrameChange's document_ready field
cb5ceec
Roman Zaynetdinov
Remove unused boolean argument from ContellationMsg's GetPipeline
Loading status checks…
95552c4
@highfive
Copy link

highfive commented Dec 25, 2016

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

highfive commented Dec 25, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/webdriver_server/lib.rs, components/constellation/constellation.rs
  • @jgraham: components/webdriver_server/lib.rs
  • @fitzgen: components/script_traits/lib.rs, components/script_traits/lib.rs
  • @KiChjang: components/script_traits/lib.rs, components/script_traits/lib.rs
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 25, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 25, 2016

📌 Commit 95552c4 has been approved by Ms2ger
@highfive highfive assigned Ms2ger and unassigned nox Dec 25, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Dec 25, 2016

⌛ Testing commit 95552c4 with merge 61d8f4d...
bors-servo added a commit that referenced this pull request Dec 25, 2016
@bors-servo
Auto merge of #14724 - zaynetro:remove-frame-change-document-ready, r…
Loading status checks…
61d8f4d 
…=Ms2ger

Remove FrameChange::document_ready

<!-- Please describe your changes on the following line: -->

Remove unused `FrameChange::document_ready` field and remove unused boolean in `ConstellationMsg::GetPipeline`'s second argument.

---
<!-- 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 #14693

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because compiler validates the field is not used anywhere

<!-- 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14724)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 25, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
@bors-servo bors-servo merged commit 95552c4 into servo:master Dec 25, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@zaynetro zaynetro deleted the zaynetro:remove-frame-change-document-ready branch Dec 25, 2016
@fflorent
fflorent reviewed Jan 2, 2017
View changes
components/constellation/constellation.rs
let frame_id = frame_id.unwrap_or(self.root_frame_id);
let current_pipeline_id = self.frames.get(&frame_id)
.map(|frame| frame.current.pipeline_id);
let current_pipeline_id_loaded = current_pipeline_id
.map(|id| (id, true));
.map(|id| id);

This comment has been minimized.

Sign in to view
@fflorent

fflorent Jan 2, 2017 •
edited

Contributor

Sorry for the late question: what this map call does? As I read it, the callback is an identity function, so it just copies the array (if I am correct).

NB: I am a beginner in Rust, so I may be wrong, don't hesitate to correct me :).

This comment has been minimized.

Sign in to view
@zaynetro

zaynetro Jan 2, 2017

Author

Good catch, it looks like it is not needed anymore. (I may be wrong as well :) )

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Jan 2, 2017

Contributor

A PR to remove current_pipeline_id_loaded would be welcome :)

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jan 3, 2017

Member

Yes, this used to do something, back when it was map(|id| (id, true)). Now it's pointless. Good catch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger

@fflorent fflorent

@asajeffrey asajeffrey

Assignees

@Ms2ger Ms2ger

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

FrameChange::document_ready is never set
7 participants
@zaynetro @highfive @Ms2ger @bors-servo @fflorent @asajeffrey @nox
You can’t perform that action at this time.