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

Send servo version in mozbrowser error. #12136

Merged
merged 1 commit into from Jul 2, 2016
Merged

Send servo version in mozbrowser error. #12136

merged 1 commit into from Jul 2, 2016

Conversation

@cbrewster
Copy link
Member

cbrewster commented Jul 1, 2016 •
edited by larsbergstrom

Adds support for sending a version string to b.html so we can put the servo version in the auto generated issue reports.

r? @asajeffrey

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12083 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because sending servo version on mozbrwosererror for issue reporter.

This change is Reviewable
@highfive
Copy link

highfive commented Jul 1, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/webidls/BrowserElement.webidl
@highfive
Copy link

highfive commented Jul 1, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@cbrewster cbrewster force-pushed the cbrewster:servo_version_reporter branch from efe3bbb to 1ce73cc Jul 1, 2016
components/script/dom/htmliframeelement.rs Outdated
@@ -354,10 +354,12 @@ impl MozBrowserEventDetailBuilder for HTMLIFrameElement {
rval.set(NullValue());
}
MozBrowserEvent::Error(error_type, description, report) => {
let version = format!("Servo {}{}", env!("CARGO_PKG_VERSION"), env!("GIT_INFO"));

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 1, 2016

Member

This could get out of sync with the version string in components/servo/main.rs - can we define it in one place and reference it from both? (Maybe util, not sure which crate is right here.)

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Jul 1, 2016

Author Member

util would make the most sense to me.
@cbrewster
Send servo version in mozbrowser error.
Loading status checks…
ed678cb 
Also moved servo version to util for usage by the --version flag
and for sending the version to browser.html with mozbrowsererror
@cbrewster cbrewster force-pushed the cbrewster:servo_version_reporter branch from 1ce73cc to ed678cb Jul 1, 2016
@asajeffrey
Copy link
Member

asajeffrey commented Jul 1, 2016

It would be nice if we could use the user agent string for this, as it's a more standardized representation of the user string. Unfortunately, the version in util/opts.rs is hardwired to be be Servo/1.0. Apart from that, this looks fine. I'll ask on irc if people are OK with including the real version number in the UA string.

Reviewed 1 of 3 files at r1, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable
@asajeffrey
Copy link
Member

asajeffrey commented Jul 1, 2016

IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=1+Jul+2016&e=1+Jul+2016#c471331

@aneeshusa and @larsbergstrom would prefer to keep the git revision out of the UA string.
@asajeffrey
Copy link
Member

asajeffrey commented Jul 1, 2016

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

bors-servo commented Jul 1, 2016

📌 Commit ed678cb has been approved by asajeffrey
@aneeshusa aneeshusa mentioned this pull request Jul 2, 2016
Closed
@asajeffrey asajeffrey mentioned this pull request Jul 2, 2016
Closed
@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2016

⌛ Testing commit ed678cb with merge ac80204...
bors-servo added a commit that referenced this pull request Jul 2, 2016
@bors-servo
Auto merge of #12136 - ConnorGBrewster:servo_version_reporter, r=asaj…
ac80204 
…effrey

Send servo version in mozbrowser error.

<!-- Please describe your changes on the following line: -->
Adds support for sending a version string to b.html so we can put the servo version in the auto generated issue reports.

r? @asajeffrey

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because sending servo version on mozbrwosererror for issue reporter.

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

bors-servo commented Jul 2, 2016

💔 Test failed - linux-rel
@cbrewster
Copy link
Member Author

cbrewster commented Jul 2, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2016

⌛ Testing commit ed678cb with merge 307844a...
bors-servo added a commit that referenced this pull request Jul 2, 2016
@bors-servo
Auto merge of #12136 - ConnorGBrewster:servo_version_reporter, r=asaj…
Loading status checks…
307844a 
…effrey

Send servo version in mozbrowser error.

<!-- Please describe your changes on the following line: -->
Adds support for sending a version string to b.html so we can put the servo version in the auto generated issue reports.

r? @asajeffrey

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because sending servo version on mozbrwosererror for issue reporter.

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

bors-servo commented Jul 2, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit ed678cb into servo:master Jul 2, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jul 2, 2016
Merged
3 of 5 tasks complete
@cbrewster cbrewster mentioned this pull request Jul 2, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Get issue reports to include the servo build id
5 participants
@cbrewster @highfive @asajeffrey @bors-servo @aneeshusa
You can’t perform that action at this time.