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

M1501: CSS Error Reporting: Final Steps Last Part #8972

Merged
merged 1 commit into from Jan 11, 2016
Merged

M1501: CSS Error Reporting: Final Steps Last Part #8972

merged 1 commit into from Jan 11, 2016

Conversation

@GauriGNaik
Copy link
Contributor

GauriGNaik commented Dec 14, 2015

Review on Reviewable
@highfive
Copy link

highfive commented Dec 14, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@jdm
Copy link
Member

jdm commented Dec 14, 2015

@GauriGNaik It would be better if you could take a recent revision from Servo's master branch, then run git cherry-pick 77e8721 and push the result instead since the other PR was merged.
@GauriGNaik GauriGNaik force-pushed the GauriGNaik:expose-css-errors-1 branch from 77e8721 to e52be03 Dec 16, 2015
components/script/dom/window.rs Outdated
@@ -1323,6 +1323,9 @@ impl Window {

WindowBinding::Wrap(runtime.cx(), win)
}
pub fn return_devtools_flag(&self) -> bool {

This comment has been minimized.

Sign in to view
@nox

nox Dec 16, 2015

Member

This function is weirdly named, what is it supposed to do?
@jdm
Copy link
Member

jdm commented Dec 16, 2015

Great! Only minor changes necessary before this can merge!
-S-awaiting-review +S-needs-code-changes

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

components/devtools/lib.rs, line 430 [r1] (raw file):
nit: indent these lines by four spaces past the let.

components/devtools/lib.rs, line 438 [r1] (raw file):
nit: unindent this line to match the D on the next line.

components/devtools_traits/lib.rs, line 90 [r1] (raw file):
Let's add /// Report a CSS parse error for the given pipeline.

components/script/dom/window.rs, line 1326 [r1] (raw file):
I'd prefer a name like live_devtools_updates instead. Also, let's unindent all these lines by 4 spaces.

components/script/script_task.rs, line 2083 [r1] (raw file):
nit: deindent this and the next line by four spaces.

components/script/script_task.rs, line 2085 [r1] (raw file):
nit: indent by two spaces.

Comments from the review on Reviewable.io
@jdm jdm self-assigned this Dec 16, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2016

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

KiChjang commented Jan 10, 2016

@GauriGNaik This PR still needs a rebase for us to merge.
@GauriGNaik
Final steps
Loading status checks…
907322c
@GauriGNaik GauriGNaik force-pushed the GauriGNaik:expose-css-errors-1 branch from fef6c69 to 907322c Jan 11, 2016
@jdm
Copy link
Member

jdm commented Jan 11, 2016

@bors-servo: r+
Thanks @GauriGNaik!
@bors-servo
Copy link
Contributor

bors-servo commented Jan 11, 2016

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

bors-servo commented Jan 11, 2016

⌛ Testing commit 907322c with merge a5a7a83...
bors-servo added a commit that referenced this pull request Jan 11, 2016
@bors-servo
Auto merge of #8972 - GauriGNaik:expose-css-errors-1, r=jdm
Loading status checks…
a5a7a83 
M1501: CSS Error Reporting: Final Steps Last Part

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8972)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 11, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 907322c into servo:master Jan 11, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@GauriGNaik @highfive @jdm @bors-servo @KiChjang @nox
You can’t perform that action at this time.