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

Implement attribute restyle hints #8381

Merged
merged 7 commits into from Nov 10, 2015
Merged

Implement attribute restyle hints #8381

merged 7 commits into from Nov 10, 2015

Conversation

@bholley
Copy link
Contributor

bholley commented Nov 6, 2015

Review on Reviewable
@highfive
Copy link

highfive commented Nov 6, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@bholley bholley force-pushed the bholley:attr_restyle_hints branch from 6365a5e to 8bdbe72 Nov 6, 2015
@bholley
Copy link
Contributor Author

bholley commented Nov 6, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2015

⌛ Trying commit 8bdbe72 with merge 9219fea...
bors-servo added a commit that referenced this pull request Nov 6, 2015
@bors-servo
Auto merge of #8381 - bholley:attr_restyle_hints, r=<try>
9219fea 
Implement attribute restyle hints

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

bors-servo commented Nov 6, 2015

💔 Test failed - mac-rel-css
@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

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

nox commented Nov 7, 2015

What is the rationale for moving AttrValue in style exactly?
@nox nox added the A-content/dom label Nov 7, 2015
components/script/dom/attr.rs
name: name,
namespace: namespace,
prefix: prefix,
},

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Nov 7, 2015

Member

Could you name this field and structure differently? foo.foo also sounded silly to me. Maybe AttrID, or AttrIdentity, or AttrInfo, or something entirely different that I can't think of?

This comment has been minimized.

Sign in to view
Copy link
@bholley

bholley Nov 9, 2015

Author Contributor

I can switch it to AttrIdentifier if you like.
components/script/dom/attr.rs Outdated
@@ -291,33 +148,41 @@ impl AttrMethods for Attr {
impl Attr {
pub fn set_value(&self, mut value: AttrValue, owner: &Element) {
assert!(Some(owner) == self.owner().r());
let notify = self.name.namespace == ns!("");
if notify {
owner.will_mutate_attr();

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Nov 7, 2015

Member

Shouldn't that be called for all namespaces, given CSS attributes selectors aren't limited to the empty namespace?

This comment has been minimized.

Sign in to view
Copy link
@bholley

bholley Nov 9, 2015

Author Contributor

Probably, yeah. I was trying to mimic the existing behavior here, but the existing behavior is probably just wrong.
@bholley
Copy link
Contributor Author

bholley commented Nov 9, 2015

What is the rationale for moving AttrValue in style exactly?
Because restyle_hints.rs needs to use it, and |style| can't depend on |script|.
This was referenced Nov 9, 2015
Closed
Closed
@bholley bholley force-pushed the bholley:attr_restyle_hints branch from 8bdbe72 to f8d0ef9 Nov 9, 2015
@highfive highfive removed the S-tests-failed label Nov 9, 2015
@bholley
Copy link
Contributor Author

bholley commented Nov 9, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

⌛ Trying commit f8d0ef9 with merge 69447d0...
bors-servo added a commit that referenced this pull request Nov 9, 2015
@bors-servo
Auto merge of #8381 - bholley:attr_restyle_hints, r=<try>
69447d0 
Implement attribute restyle hints

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

bors-servo commented Nov 9, 2015

💔 Test failed - mac-rel-css
@bholley bholley force-pushed the bholley:attr_restyle_hints branch from f8d0ef9 to 9e575af Nov 9, 2015
@highfive highfive removed the S-tests-failed label Nov 9, 2015
@bholley bholley force-pushed the bholley:attr_restyle_hints branch from 9e575af to f6e03a8 Nov 9, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Nov 10, 2015

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

bors-servo commented Nov 10, 2015

📌 Commit 3e0e806 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

⌛ Testing commit 3e0e806 with merge 1c7c0e4...
bors-servo added a commit that referenced this pull request Nov 10, 2015
@bors-servo
Auto merge of #8381 - bholley:attr_restyle_hints, r=pcwalton
1c7c0e4 
Implement attribute restyle hints

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

pcwalton commented Nov 10, 2015

@bors-servo: retry try- r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

📌 Commit 3e0e806 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

⌛ Testing commit 3e0e806 with merge 3f2fb58...
bors-servo added a commit that referenced this pull request Nov 10, 2015
@bors-servo
Auto merge of #8381 - bholley:attr_restyle_hints, r=pcwalton
3f2fb58 
Implement attribute restyle hints

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

Manishearth commented Nov 10, 2015 

./components/style/restyle_hints.rs:292: Line is longer than 120 characters
bholley added 4 commits Nov 5, 2015
@bholley
Implement attribute restyle hints.
7fa7936 
Fixes #6942.
@bholley
Add ::from_layout_js factory to LayoutNode, LayoutElement, and Layout…
b293692 
…Document.
@bholley
Tests.
a79f6e4
@bholley
Update test expectations.
Loading status checks…
5d85956
@bholley bholley force-pushed the bholley:attr_restyle_hints branch from 3e0e806 to 5d85956 Nov 10, 2015
@Manishearth
Copy link
Member

Manishearth commented Nov 10, 2015

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

bors-servo commented Nov 10, 2015

📌 Commit 5d85956 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

⌛ Testing commit 5d85956 with merge 13226f8...
bors-servo added a commit that referenced this pull request Nov 10, 2015
@bors-servo
Auto merge of #8381 - bholley:attr_restyle_hints, r=pcwalton
Loading status checks…
13226f8 
Implement attribute restyle hints

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

bors-servo commented Nov 10, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 5d85956 into servo:master Nov 10, 2015
1 of 3 checks passed
1 of 3 checks passed
code-review/reviewable Review in progress: 0 of 18 files reviewed, 2 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
@bholley bholley deleted the bholley:attr_restyle_hints branch Oct 30, 2016
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
A-content/dom S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@bholley @highfive @bors-servo @nox @jdm @pcwalton @Manishearth
You can’t perform that action at this time.