Skip to content

Pipeline visibility logic change#13227

Merged
bors-servo merged 1 commit intoservo:masterfrom
jmr0:pipeline_visibility
Sep 19, 2016
Merged

Pipeline visibility logic change#13227
bors-servo merged 1 commit intoservo:masterfrom
jmr0:pipeline_visibility

Conversation

@jmr0
Copy link
Copy Markdown
Contributor

@jmr0 jmr0 commented Sep 12, 2016 •
edited by larsbergstrom
Loading

cc @paulrouget @jdm

  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link
Copy Markdown

highfive commented Sep 12, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 12, 2016
@paulrouget
Copy link
Copy Markdown
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
Copy Markdown
Contributor

bors-servo commented Sep 14, 2016

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Sep 14, 2016
@jdm
Copy link
Copy Markdown
Member

jdm commented Sep 14, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned pcwalton Sep 14, 2016
@jdm
Copy link
Copy Markdown
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 There is new code that needs to be reviewed. label Sep 17, 2016
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 17, 2016
@paulrouget
Copy link
Copy Markdown
Contributor

paulrouget commented Sep 19, 2016

Works for me :)

@KiChjang KiChjang removed the S-needs-rebase There are merge conflict errors. label Sep 19, 2016
@KiChjang
Copy link
Copy Markdown
Contributor

KiChjang commented Sep 19, 2016

@bors-servo r=jdm

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Sep 19, 2016

📌 Commit cf21ea5 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 19, 2016
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Sep 19, 2016

⌛ Testing commit cf21ea5 with merge 157e3cc...

bors-servo pushed a commit that referenced this pull request Sep 19, 2016
Auto merge of #13227 - jmr0:pipeline_visibility, r=jdm
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
Copy Markdown
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
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 19, 2016
This was referenced Sep 19, 2016
Merged
Closed
Closed
Closed
@jmr0
Copy link
Copy Markdown
Contributor Author

jmr0 commented Sep 19, 2016

Thanks @paulrouget and @jdm

@jmr0 jmr0 deleted the pipeline_visibility branch September 19, 2016 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm jdm approved these changes

Assignees

@jdm jdm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

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