Current pixel density #21628

paavininanda · 2018-09-06T16:43:24Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix a subset of #11416.

There are tests for these changes OR

This change is

highfive · 2018-09-06T16:43:29Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/htmlimageelement.rs, components/script/dom/window.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/htmlimageelement.rs, components/script/dom/window.rs
@emilio: components/layout/fragment.rs, components/layout/construct.rs

highfive · 2018-09-06T16:43:31Z

Warning

These commits modify unsafe code. Please review it carefully!

paavininanda · 2018-09-06T16:44:22Z

r? @jdm

jdm

Looking good!

jdm · 2018-09-06T19:04:47Z

components/layout/fragment.rs

+        let mut current_pixel_density = 1 as f64;
+        if density.is_some() {
+            current_pixel_density = density.unwrap();
+        }


let current_pixel_density = density.unwrap_or(1.0);

components/layout/fragment.rs

jdm · 2018-09-06T19:04:47Z

components/layout/fragment.rs

+            Some(ImageOrMetadataAvailable::ImageAvailable(i, _)) => {
+                (
+                    Some(Arc::new(Image {
+                        height: (i.height as f64 / current_pixel_density) as u32,


Let's store these values in local variables so we don't have to repeat the calculations.

jdm · 2018-09-06T19:04:47Z

components/layout/fragment.rs

+                        width: (i.width as f64 / current_pixel_density) as u32,
+                    }),
+                )
+            },
            Some(ImageOrMetadataAvailable::MetadataAvailable(m)) => (None, Some(m)),


We should probably perform the density calculation here as well.

jdm · 2018-09-06T19:04:47Z

components/script/dom/htmlimageelement.rs

@@ -706,7 +706,7 @@ impl HTMLImageElement {
    }

    /// Step 13-17 of html.spec.whatwg.org/multipage/#update-the-image-data
-    fn prepare_image_request(&self, url: &ServoUrl, src: &DOMString) {
+    fn prepare_image_request(&self, url: &ServoUrl, src: &DOMString, current_pixel_density: Option<f64>) {


Let's call this selected_pixel_density, and we should accept a non-option value.

jdm · 2018-09-06T19:04:47Z

components/script/dom/htmlimageelement.rs

-                src.0
-            },
+                data
+            }


nit: Some(data) => data,

components/script/dom/htmlimageelement.rs

jdm · 2018-09-06T19:04:47Z

components/script/dom/htmlimageelement.rs

            None => 0,
        }
    }

    // https://html.spec.whatwg.org/multipage/#dom-img-naturalheight
    fn NaturalHeight(&self) -> u32 {
        let ref metadata = self.current_request.borrow().metadata;
+        let pixel_density = (*self).current_request.borrow().current_pixel_density.clone().unwrap_or(1f64);


Same as previous comment.

jdm · 2018-09-06T19:04:47Z

components/layout/construct.rs

@@ -1471,6 +1473,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
            ImageUrlOrNone::Url(ref url_value) => {
                let image_info = Box::new(ImageFragmentInfo::new(
                    url_value.url().map(|u| u.clone()),
+                    node.image_density(),


I expect this to break because the node won't be an image element. We should just pass None here.

jdm · 2018-09-06T19:04:47Z

components/layout/construct.rs

@@ -408,6 +409,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
            Some(LayoutNodeType::Element(LayoutElementType::HTMLObjectElement)) => {
                let image_info = Box::new(ImageFragmentInfo::new(
                    node.object_data(),
+                    node.image_density(),


This will break because the node is not an image element. We should pass None here.

jdm · 2018-09-06T21:13:35Z

This can merge after #21280 merges.

bors-servo · 2018-09-12T17:35:36Z

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


        Current-pixel-density tests passing

jdm · 2018-09-12T18:56:12Z

@bors-servo r+

bors-servo · 2018-09-12T18:56:13Z

📌 Commit 25027e4 has been approved by jdm

bors-servo · 2018-09-12T22:16:14Z

⌛ Testing commit 25027e4 with merge 44394a5...


        Auto merge of #21628 - paavininanda:Current-pixel-density, r=jdm

Current pixel density  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix a subset of #11416.  - [x] There are tests for these changes OR    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21628)

bors-servo · 2018-09-12T22:36:14Z

💔 Test failed - linux-rel-css

jdm · 2018-09-12T22:46:00Z

@bors-servo retry

#21698

bors-servo · 2018-09-12T22:57:36Z

⚡ Previous build results for android, android-x86, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4 are reusable. Rebuilding only linux-rel-css, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, windows-msvc-dev...

bors-servo · 2018-09-12T23:11:00Z

💔 Test failed - linux-rel-css

jdm · 2018-09-13T02:56:14Z

@bors-servo retry

#13480

bors-servo · 2018-09-13T04:16:48Z

⌛ Testing commit 25027e4 with merge b0de9c5...


        Auto merge of #21628 - paavininanda:Current-pixel-density, r=jdm

Current pixel density  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix a subset of #11416.  - [x] There are tests for these changes OR    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21628)

bors-servo · 2018-09-13T05:13:07Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing b0de9c5 to master...

highfive assigned avadacatavra Sep 6, 2018

highfive added the S-awaiting-review label Sep 6, 2018

highfive assigned jdm and unassigned avadacatavra Sep 6, 2018

jdm requested changes Sep 6, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 6, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 6, 2018

paavininanda force-pushed the paavininanda:Current-pixel-density branch from 080cb99 to ada19d8 Sep 6, 2018

jdm approved these changes Sep 6, 2018

View changes

jdm added S-blocked-on-external and removed S-awaiting-review labels Sep 6, 2018

highfive added the S-needs-rebase label Sep 12, 2018

jdm removed the S-blocked-on-external label Sep 12, 2018

paavininanda force-pushed the paavininanda:Current-pixel-density branch from ada19d8 to be50a80 Sep 12, 2018

highfive added the S-awaiting-review label Sep 12, 2018

Current-pixel-density tests passing

Loading status checks…

25027e4

paavininanda force-pushed the paavininanda:Current-pixel-density branch from be50a80 to 25027e4 Sep 12, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 12, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 12, 2018

highfive added the S-tests-failed label Sep 12, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 13, 2018

bors-servo merged commit 25027e4 into servo:master Sep 13, 2018
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 13, 2018

servo / servo

Current pixel density #21628

Current pixel density #21628

paavininanda commented Sep 6, 2018 •

edited by SimonSapin

highfive commented Sep 6, 2018

highfive commented Sep 6, 2018

paavininanda commented Sep 6, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 6, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 13, 2018

bors-servo commented Sep 13, 2018

bors-servo commented Sep 13, 2018

servo / servo

Join GitHub today

Current pixel density #21628

Current pixel density #21628

Conversation

paavininanda commented Sep 6, 2018 • edited by SimonSapin

highfive commented Sep 6, 2018

highfive commented Sep 6, 2018

paavininanda commented Sep 6, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 6, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 12, 2018

bors-servo commented Sep 12, 2018

bors-servo commented Sep 12, 2018

jdm commented Sep 13, 2018

bors-servo commented Sep 13, 2018

bors-servo commented Sep 13, 2018

paavininanda commented Sep 6, 2018 •

edited by SimonSapin