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

Pipeline visibility logic change #13227

Merged
merged 1 commit into from Sep 19, 2016
Merged

Pipeline visibility logic change #13227

merged 1 commit into from Sep 19, 2016

Conversation

@jmr0
Copy link
Contributor

jmr0 commented Sep 12, 2016 •
edited by larsbergstrom

cc @paulrouget @jdm

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13191 (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 Sep 12, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs
@paulrouget
Copy link
Contributor

paulrouget commented Sep 12, 2016

Thanks a lot for taking care of this!

The notion of "previous"/"old" pipeline is not always clear. For example, on reload, a new pipeline is created and the current pipeline is dismissed (see #13167). What's the meaning of old/previous in this case?

Because this might be confusing, I would recommend to pass the visible:bool to new_pipeline() instead of old_pipeline, and let the caller figure out if the new pipeline is visible or not (reload will use the visibility of the pipeline it's replacing, navigate will use the previous-in-history pipeline, init load will use its parent pipeline).

As usual, I'm not a reviewer, I'll let someone else comment on this.
@bors-servo
Copy link
Contributor

bors-servo commented Sep 14, 2016

☔ The latest upstream changes (presumably #11698) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm
Copy link
Member

jdm commented Sep 14, 2016

r? @jdm
@highfive highfive assigned jdm and unassigned pcwalton Sep 14, 2016
@jdm
Copy link
Member

jdm commented Sep 17, 2016

I found @paulrouget's comments valuable to help focus on the intended effects of these changes, but I actually disagree with his conclusion. I think providing what is effectively a source pipeline to new_pipeline is cleaner; I don't want each caller to have to figure out what visibility should be passed. Accordingly, I'm happy to merge these changes once they're rebased. Thank you for dealing with this!
@jdm jdm removed the S-awaiting-review label Sep 17, 2016
@jdm
jdm approved these changes Sep 17, 2016
View changes
@jmr0 jmr0 force-pushed the jmr0:pipeline_visibility branch from eb7a9c4 to df3d7cf Sep 17, 2016
@jmr0
altering pipeline visibility change logic
Loading status checks…
cf21ea5
@jmr0 jmr0 force-pushed the jmr0:pipeline_visibility branch from df3d7cf to cf21ea5 Sep 17, 2016
@paulrouget
Copy link
Contributor

paulrouget commented Sep 19, 2016

Works for me :)
@KiChjang
Copy link
Member

KiChjang commented Sep 19, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2016

📌 Commit cf21ea5 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2016

⌛ Testing commit cf21ea5 with merge 157e3cc...
bors-servo added a commit that referenced this pull request Sep 19, 2016
@bors-servo
Auto merge of #13227 - jmr0:pipeline_visibility, r=jdm
Loading status checks…
157e3cc 
Pipeline visibility logic change

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

cc @paulrouget @jdm

---
<!-- 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 #13191 (github issue number if applicable).

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

bors-servo commented Sep 19, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit cf21ea5 into servo:master Sep 19, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Sep 19, 2016
Merged
Closed
Closed
@bors-servo bors-servo mentioned this pull request Sep 19, 2016
Closed
@jmr0
Copy link
Contributor Author

jmr0 commented Sep 19, 2016

Thanks @paulrouget and @jdm
@jmr0 jmr0 deleted the jmr0:pipeline_visibility branch Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

The pipeline visibility logic is wrong
7 participants
@jmr0 @highfive @paulrouget @bors-servo @jdm @KiChjang @pcwalton
You can’t perform that action at this time.