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

Completed implementation of devtools' `getLayout`. #9661

Merged
merged 1 commit into from Feb 24, 2016
Merged

Completed implementation of devtools' `getLayout`. #9661

merged 1 commit into from Feb 24, 2016

Conversation

@jdm
Copy link
Member

jdm commented Feb 16, 2016

Rebase of #7267. Fixes #3598.

This avoids all of the sketchy issues of trying to read the style data for margins from the script thread. I replaced it with a layout query that fetches the margin style properties for a given element.

Review on Reviewable
@highfive
Copy link

highfive commented Feb 16, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout and script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member Author

jdm commented Feb 16, 2016

r? @pcwalton
@jdm jdm mentioned this pull request Feb 16, 2016
Closed
@emilio
Copy link
Member

emilio commented Feb 17, 2016

A lot of nits, over all from the serde(rename="") and a conflicting dep.

./components/devtools/actors/inspector.rs:407: missing space before =
./components/devtools/actors/inspector.rs:407: missing space after =
./components/devtools/actors/inspector.rs:409: missing space before =
./components/devtools/actors/inspector.rs:409: missing space after =
./components/devtools/actors/inspector.rs:415: missing space before =
./components/devtools/actors/inspector.rs:415: missing space after =
./components/devtools/actors/inspector.rs:417: missing space before =
./components/devtools/actors/inspector.rs:417: missing space after =
./components/devtools/actors/inspector.rs:419: missing space before =
./components/devtools/actors/inspector.rs:419: missing space after =
./components/devtools/actors/inspector.rs:421: missing space before =
./components/devtools/actors/inspector.rs:421: missing space after =
./components/devtools/actors/inspector.rs:424: missing space before =
./components/devtools/actors/inspector.rs:424: missing space after =
./components/devtools/actors/inspector.rs:426: missing space before =
./components/devtools/actors/inspector.rs:426: missing space after =
./components/devtools/actors/inspector.rs:428: missing space before =
./components/devtools/actors/inspector.rs:428: missing space after =
./components/devtools/actors/inspector.rs:430: missing space before =
./components/devtools/actors/inspector.rs:430: missing space after =
./components/devtools/actors/inspector.rs:433: missing space before =
./components/devtools/actors/inspector.rs:433: missing space after =
./components/devtools/actors/inspector.rs:435: missing space before =
./components/devtools/actors/inspector.rs:435: missing space after =
./components/devtools/actors/inspector.rs:437: missing space before =
./components/devtools/actors/inspector.rs:437: missing space after =
./components/devtools/actors/inspector.rs:439: missing space before =
./components/devtools/actors/inspector.rs:439: missing space after =
./components/script/devtools.rs:155: missing space before =
./components/servo/Cargo.lock:1: duplicate versions for package "serde_json"
    found dependency on version 0.5.1
    but highest version is 0.6.0
    try upgrading with ./mach cargo-update -p serde_json:0.5.1
    The following packages depend on version 0.5.1:
        layout
@jdm jdm added the S-fails-tidy label Feb 17, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2016

☔ The latest upstream changes (presumably #9603) made this pull request unmergeable. Please resolve the merge conflicts.
@pcwalton
Copy link
Contributor

pcwalton commented Feb 24, 2016

r=me when rebased.
@benschulz @jdm
Completed implementation of devtools' `getLayout`.
Loading status checks…
0785d91
@jdm jdm force-pushed the jdm:devtools-inspector-get-layout branch from 96b1ca9 to 0785d91 Feb 24, 2016
@jdm
Copy link
Member Author

jdm commented Feb 24, 2016

@bors-servo: r=pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2016

📌 Commit 0785d91 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2016

⌛ Testing commit 0785d91 with merge 09e987b...
bors-servo added a commit that referenced this pull request Feb 24, 2016
@bors-servo
Auto merge of #9661 - jdm:devtools-inspector-get-layout, r=pcwalton
Loading status checks…
09e987b 
Completed implementation of devtools' `getLayout`.

Rebase of #7267. Fixes #3598.

This avoids all of the sketchy issues of trying to read the style data for margins from the script thread. I replaced it with a layout query that fetches the margin style properties for a given element.

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

bors-servo commented Feb 24, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 0785d91 into servo:master Feb 24, 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 Feb 24, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pcwalton pcwalton

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
@jdm @highfive @emilio @bors-servo @pcwalton @benschulz
You can’t perform that action at this time.