Skip to content
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

Use image metadata to lay out images #9208

Merged
merged 1 commit into from Jan 30, 2016
Merged

Use image metadata to lay out images #9208

merged 1 commit into from Jan 30, 2016

Conversation

@jmr0
Copy link
Contributor

jmr0 commented Jan 8, 2016

Fixes #7047.

cc: @jdm

Review on Reviewable

@highfive
Copy link

highfive commented Jan 8, 2016

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@jmr0 jmr0 changed the title Master img Use image metadata to lay out images Jan 8, 2016
@jdm jdm self-assigned this Jan 8, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2016

The latest upstream changes (presumably #9201) made this pull request unmergeable. Please resolve the merge conflicts.

@jmr0 jmr0 force-pushed the jmr0:master-img branch from 694cdd7 to 0ec3406 Jan 10, 2016
@asajeffrey asajeffrey assigned asajeffrey and unassigned jdm Jan 26, 2016
@asajeffrey
Copy link
Member

asajeffrey commented Jan 26, 2016

Code reviewed, it looks pretty sweet. Only one niggle, which is that metadata width/height are being kept as u32, in px. Is there a sensible way of tracking units for metadata?


Reviewed 12 of 13 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/msg/constellation_msg.rs, line 334 [r2] (raw file):
Should image metadata width/height be kept in a type that specifies units?


Comments from the review on Reviewable.io

@jmr0
Copy link
Contributor Author

jmr0 commented Jan 28, 2016

Great to hear! Are you thinking of something like a Pixel type to abstract the u32 choice? In that case it might be worthwhile changing Image's width/height from u32 as well

@asajeffrey
Copy link
Member

asajeffrey commented Jan 28, 2016

On reflection, that issue is orthogonal to your PR, so I think we are good to go.

@jdm: Can you delegate review privilege to me?

@jdm
Copy link
Member

jdm commented Jan 28, 2016

@bors-servo: delegate=asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

✌️ @asajeffrey can now approve this pull request

@asajeffrey
Copy link
Member

asajeffrey commented Jan 28, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

📌 Commit e421595 has been approved by asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

Testing commit e421595 with merge 391b741...

bors-servo added a commit that referenced this pull request Jan 28, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

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

bors-servo commented Jan 28, 2016

💔 Test failed - gonk

@jdm
Copy link
Member

jdm commented Jan 28, 2016

Ah, looks like this requires running ./mach cargo-update -p immeta to ensure that all of the Cargo.lock files in the tree are properly updated with the new package.

@jmr0 jmr0 force-pushed the jmr0:master-img branch from e421595 to 019429b Jan 29, 2016
bors-servo added a commit that referenced this pull request Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

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

bors-servo commented Jan 30, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Jan 30, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2016

Testing commit 167ffa7 with merge 196c816...

bors-servo added a commit that referenced this pull request Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

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

bors-servo commented Jan 30, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Jan 30, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2016

Testing commit 167ffa7 with merge 46b3eb6...

bors-servo added a commit that referenced this pull request Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

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

bors-servo commented Jan 30, 2016

@bors-servo bors-servo merged commit 167ffa7 into servo:master Jan 30, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jmr0 jmr0 deleted the jmr0:master-img branch Jan 31, 2016
mbrubeck added a commit to mbrubeck/servo that referenced this pull request Feb 25, 2016
PR servo#9208 added the ability to perform layout before an `<img>` resource is
fully loaded. However, when we are rendering to an image file for testing
purposes, we need to block until the image content is fully loaded (until
issue servo#9441 is fixed).

Fixes servo#9550.
mbrubeck added a commit to mbrubeck/servo that referenced this pull request Feb 25, 2016
PR servo#9208 added the ability to perform layout before an `<img>` resource is
fully loaded. However, when we are rendering to an image file for testing
purposes, we need to block until the image content is fully loaded (until
issue servo#9441 is fixed).

Fixes servo#9550.
mbrubeck added a commit to mbrubeck/servo that referenced this pull request Feb 25, 2016
PR servo#9208 added the ability to perform layout before an `<img>` resource is
fully loaded. However, when we are rendering to an image file for testing
purposes, we need to block until the image content is fully loaded (until
issue servo#9441 is fixed).

Fixes servo#9550.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.