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

Setting a devtools timeline marker may fail, due to pipeline lookup failure #13728

Merged
merged 1 commit into from Oct 13, 2016
Merged

Setting a devtools timeline marker may fail, due to pipeline lookup failure #13728

merged 1 commit into from Oct 13, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Oct 12, 2016 •
edited by larsbergstrom

Allow setting a devtools timeline marker to fail, due to pipeline lookup failure. This is part of tidying up pipeline lookup.

cc @jdm

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because I'm not sure how to test devtools.

This change is Reviewable
@asajeffrey
Setting a devtools timeline marker may fail, due to pipeline lookup f…
Loading status checks…
a6d83a3 
…ailure.
@highfive
Copy link

highfive commented Oct 12, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/devtools/actors/timeline.rs, components/devtools_traits/lib.rs, components/devtools_traits/lib.rs
  • @KiChjang: components/script/script_thread.rs, components/script/dom/window.rs, components/script/devtools.rs
@highfive
Copy link

highfive commented Oct 12, 2016

warning Warning warning

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

asajeffrey commented Oct 12, 2016

r? @fitzgen
@highfive highfive assigned fitzgen and unassigned metajack Oct 12, 2016
@fitzgen
fitzgen approved these changes Oct 12, 2016
View changes
Copy link
Member

fitzgen left a comment

r=me with question about whether Option is necessary (seems to me that it isn't).

@bors-servo r+ (is this still needed now that you can mark approval in github pull request reviews?)
components/devtools_traits/lib.rs
@@ -190,6 +190,7 @@ pub struct AutoMargins {
}

/// Messages to process in a particular script thread, as instructed by a devtools client.
/// TODO: better error handling, e.g. if pipeline id lookup fails?

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Oct 12, 2016

Member

File an issue for this TODO and make this comment "TODO #NNNNN: ..."
components/script/devtools.rs
window.set_devtools_timeline_markers(marker_types, reply);
reply: IpcSender<Option<TimelineMarker>>) {
match context.find(pipeline) {
None => reply.send(None).unwrap(),

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Oct 12, 2016

Member

What use does the receiver have for a None? What if we only sent the timeline marker if we find the pipeline, and therefore still have IpcSender<TimelineMarker> instead of introducing an Option in there?

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Oct 12, 2016

Member

Yeah, it seems to me that the Option isn't carrying its weight since the receiver is giving up as soon as it gets a None in the while let Ok(Some(...)) = ... loop.
@fitzgen
Copy link
Member

fitzgen commented Oct 12, 2016

@bors-servo r+
@KiChjang
Copy link
Member

KiChjang commented Oct 12, 2016

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

bors-servo commented Oct 12, 2016

📌 Commit a6d83a3 has been approved by fitzgen
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 12, 2016

@fitzgen there's already #13161, would that do as an issue?

The reason for using am Option is to signal failure early, I'm a bit worried that if we did this just by dropping the channel that we'd end up with future changes to the console causing intermittent deadlocks.
@fitzgen
Copy link
Member

fitzgen commented Oct 12, 2016

@fitzgen there's already #13161, would that do as an issue?

I suppose so, not 100% clear what the original issue's intent is.

The reason for using am Option is to signal failure early, I'm a bit worried that if we did this just by dropping the channel that we'd end up with future changes to the console causing intermittent deadlocks.

Ok, fine by me.
@asajeffrey asajeffrey mentioned this pull request Oct 12, 2016
Closed
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2016

⌛ Testing commit a6d83a3 with merge bb75e2e...
bors-servo added a commit that referenced this pull request Oct 13, 2016
@bors-servo
Auto merge of #13728 - asajeffrey:devtools-set-timeline-marker-may-fa…
Loading status checks…
bb75e2e 
…il, r=fitzgen

Setting a devtools timeline marker may fail, due to pipeline lookup failure

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

Allow setting a devtools timeline marker to fail, due to pipeline lookup failure. This is part of tidying up pipeline lookup.

cc @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 do not require tests because I'm not sure how to test devtools.

<!-- 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/13728)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit a6d83a3 into servo:master Oct 13, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen

Assignees

@fitzgen fitzgen

Labels
A-devtools
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@asajeffrey @highfive @fitzgen @KiChjang @bors-servo @metajack
You can’t perform that action at this time.