ImageMaps: Implemented support for parsing coord attribute to a shape… #13972

shravan-achar · 2016-10-29T04:58:03Z

Image Maps: (Part 1) Implemented support for parsing coord attribute to a shape object.
Implemented a hit_test method to see if a point is within the area. Tests for constructors and hit_test included

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

… object in HTMLAreaElement

This change is

emilio

Mostly superficial review, but there are a lot of style nits, so I hope it'd be helpful.

Thanks for doing this work!

emilio · 2016-10-29T20:22:23Z

components/script/dom/htmlareaelement.rs

@@ -35,9 +157,16 @@ impl HTMLAreaElement {
               prefix: Option<DOMString>,
               document: &Document) -> Root<HTMLAreaElement> {
        Node::reflect_node(box HTMLAreaElement::new_inherited(local_name, prefix, document),
-                           document,
-                           HTMLAreaElementBinding::Wrap)
+        document,


Why this indentation change?

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+    let input = "1.1, 1.1, 6.1, 1.1, 3.1, 3.1";
+    let size = 6;
+
+    let mut vec = Vec::new ();


nit: rust style is not having space before the parentheses in a function call.

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+/*Area::Rectangle tests*/
+#[test]
+fn polygon_six_points_valid_input ()
+{


nit: Brace uses to be in the same line as the function name except when there's a where clause.

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+
+/*Area::Rectangle tests*/
+#[test]
+fn polygon_six_points_valid_input ()


nit: no space after function name

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+
+    let mut vec = Vec::new ();
+
+    vec.push(1.1);


Also, you could write this as: vec![1.1, 1.1, 6.1, ...], here and everywhere else.

emilio · 2016-10-29T20:22:23Z

components/script/dom/htmlareaelement.rs

+            }
+
+            //only junk exist
+            if array.len() == 0 {


nit: array.is_empty().

emilio · 2016-10-29T20:22:23Z

components/script/dom/htmlareaelement.rs

+                index = index + 1;
+            }
+
+            //only junk exist


nit: Only junk exists.

emilio · 2016-10-29T20:22:23Z

components/script/dom/htmlareaelement.rs

+    {
+        let num;
+        let size;
+        let mut stringval;


nit: please use snake_case: string_val, number_list.

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+            assert_eq! (left_r, vec[2]);
+            assert_eq! (top_b, vec[3]);
+        },
+        _ => { assert!(false); },


You can use panic! here too if you want, but that's probably fine.

unreachable!() may make more sense here.

emilio · 2016-10-29T20:22:23Z

tests/unit/script/htmlareaelement.rs

+        Area::Rectangle { left_l, top_t, left_r, top_b } => {
+            assert!(left_l < left_r);
+            assert!(top_t < top_b);
+            assert_eq! (left_l, vec[0]);


nit: No space after assert_eq!.

emilio · 2016-10-29T20:22:36Z

r? @jdm

shravan-achar · 2016-10-31T23:37:16Z

Thanks Emilio and Keith for your comments. Changes made as per comments.

jdm

Great start! The comments I have are a combination of style nits and suggestions for more idiomatic Rust code, along with some design changes that should make the resulting code easier to read.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

-                           document,
-                           HTMLAreaElementBinding::Wrap)
+               document,
+               HTMLAreaElementBinding::Wrap)


nit: revert this indentation change.

This should still be reverted.

jdm · 2016-11-01T15:24:01Z

components/servo/Cargo.lock

@@ -2017,6 +2017,7 @@ dependencies = [
 name = "script_tests"
 version = "0.0.1"
 dependencies = [
+ "euclid 0.10.2 (registry+https://github.com/rust-lang/crates.io-index)",


We will need to run ./mach cargo-update -p script to ensure that ports/cef/Cargo.lock is also updated.

The command throws an error: package id specification script matched no package. Is it a different package name?

Odd - what about ./mach cargo-update -p script_tests?

No luck. Same error. Tried script_tests, script, tests, test, scripts too.

I'm not sure why that's happening, but I think my original comment was wrong and ports/cef/Cargo.lock does not need to be updated. Go ahead and ignore this.

jdm · 2016-11-01T15:24:01Z

tests/unit/script/Cargo.toml

@@ -14,3 +14,4 @@ msg = {path = "../../../components/msg"}
 plugins = {path = "../../../components/plugins"}
 script = {path = "../../../components/script"}
 url = {version = "1.2", features = ["heap_size"]}
+euclid = "0.10.2"


nit: keep these in alphabetical order, please.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+    Circle { left: f32, top: f32, radius: f32 },
+    Rectangle { left_l: f32, top_t: f32, left_r: f32, top_b: f32 },
+    Polygon { points: Vec<f32> },
+    Default,


Rather than a default variant, we should make our parsing functions return Option<Area> instead.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+
+// https://html.spec.whatwg.org/multipage/#rules-for-parsing-a-list-of-floating-point-numbers
+impl Area {
+    pub fn get_area(coord: &str) -> Area {


Let's call this method parse and have it return Option<Area>.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+
+        let final_size = number_list.len();
+
+        if final_size == 3 {


The specification says that we should parse lists of coordinates with a particular target state. From "each area element in areas must be processed as follows", see step 4: https://html.spec.whatwg.org/multipage/embedded-content.html#image-map-processing-model . Rather than only returning a circle if there are exactly 3 elements, we should be returning a circle if there are at least 3 instead (and we're attempting to parse a circle).

I recommend a second enum with variants for the intended parse target, and taking that as an argument in Area::parse.

In HTMLAreaElement.idl, we noticed that all the attributes are commented out. We were thinking if these are necessary to access 'area' elements attributes such as shape and coords. We were not sure how to access these
For this, we were thinking of seeing which of the values are populated by running a debugger on servo, but we were also not sure of which of the HTML features are implemented on Servo.

The attributes in the webidl files only affect JS code that uses the element.something syntax. It's still possible to use element.getAttribute('something'), which corresponds to the <element something=value> content attribute. Does that make sense?

Oh, and if you were really asking about how to access the attribute value from Servo: https://dxr.mozilla.org/servo/rev/ebae89aa2e7d83801d8b0a3ec83ba208b551663d/components/script/dom/htmlmediaelement.rs#446 is a good example.

You may need to use Atom::from_slice("coords") instead of atom!("coords"), because it's not present in the list in https://github.com/servo/string-cache/ yet.

We get the 'shape' attribute as a string. Do we need another enum to state the variant or we can pass around the string (ex: parse("1,1,1,1", "Rectangle"))?

I think it makes sense to pass a strongly-typed enum rather than a string.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+            }
+
+            Area::Rectangle { left_l: number_list[0], top_t: number_list[1], left_r: number_list[2],
+                              top_b: number_list[3] }


nit:

Area::Rectangle { left_l: ..., top_t: ..., // etc. }

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+                                                  radius * radius <= 0.0,
+
+            Area::Rectangle { left_l, top_t, left_r, top_b } => p.x <= left_r && p.x >= left_l &&
+                                                                p.y <= top_b && p.y >= top_t,


nit:

Area::Rectangle { left_l, top_t, left_r, top_b } => p.x <= left_r && p.x >= left_l && p.y <= top_b && p.y >= top_t,

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+
+    pub fn hit_test(&self, p: Point2D<f32>) -> bool {
+        match *self {
+            Area::Circle { left, top, radius } => (p.x - left)*(p.x - left) +


nit: move the calculation on to the next line.

jdm · 2016-11-01T15:24:01Z

components/script/dom/htmlareaelement.rs

+            }
+
+            // Convert String to float
+            let mut string_val = String::from_utf8(array);


I don' think this needs to be mutable?

bors-servo · 2016-11-03T18:05:05Z

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

jdm · 2016-11-04T17:33:04Z

./mach test-tidy is failing:

./components/script/dom/htmlareaelement.rs:16: use statement is not in alphabetical order
    expected: html5ever_atoms::LocalName
    found: euclid::point::Point2D

shravan-achar · 2016-11-05T03:46:52Z

Changes made. Test-tidy and build pass.

shravan-achar · 2017-01-11T05:12:27Z

Minor nits fixed


        HTML image map support

Manishearth · 2017-01-11T09:30:45Z

@bors-servo r=Manishearth,emilio,jdm

bors-servo · 2017-01-11T09:30:46Z

📌 Commit 980dde7 has been approved by Manishearth,emilio,jdm

bors-servo · 2017-01-11T09:30:50Z

⌛ Testing commit 980dde7 with merge a065191...


        Auto merge of #13972 - shravan-achar:master, r=Manishearth,emilio,jdm

ImageMaps: Implemented support for parsing coord attribute to a shape…  Image Maps: (Part 1) Implemented support for parsing coord attribute to a shape object. Implemented a hit_test method to see if a point is within the area. Tests for constructors and hit_test included ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  … object in HTMLAreaElement  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13972)

bors-servo · 2017-01-11T09:46:56Z

💔 Test failed - linux-dev

Manishearth · 2017-01-11T16:08:26Z

@jdm hmm, the unit tests here try to use script::dom, which is private. We've never really had unit tests for script, so we haven't hit this problem before. Any idea how this should be addressed?

emilio · 2017-01-11T16:18:35Z

See components/script/test.rs, where we re-export the DOM things that need to be testable.

…

On Wed, Jan 11, 2017 at 08:08:33AM -0800, Manish Goregaokar wrote: @jdm hmm, the unit tests here try to use `script::dom`, which is private. We've never really had unit tests for script, so we haven't hit this problem before. Any idea how this should be addressed? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #13972 (comment)

jdm · 2017-01-11T17:53:56Z

Sorry @shravan-achar, #11672 merged since these changes were made which caused the build error when compiling tests.


        Make area structs accessible from area tests

Manishearth · 2017-01-11T20:24:44Z

@bors-servo r=Manishearth,emilio,jdm

bors-servo · 2017-01-11T20:24:44Z

📌 Commit d3be70b has been approved by Manishearth,emilio,jdm

bors-servo · 2017-01-11T20:25:48Z

⌛ Testing commit d3be70b with merge f6940f6...


        Auto merge of #13972 - shravan-achar:master, r=Manishearth,emilio,jdm

ImageMaps: Implemented support for parsing coord attribute to a shape…  Image Maps: (Part 1) Implemented support for parsing coord attribute to a shape object. Implemented a hit_test method to see if a point is within the area. Tests for constructors and hit_test included ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  … object in HTMLAreaElement  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13972)

bors-servo · 2017-01-11T21:23:04Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

shravan-achar force-pushed the shravan-achar:master branch from 489ab18 to 010e606 Oct 29, 2016

highfive added the S-awaiting-review label Oct 29, 2016

emilio requested changes Oct 29, 2016

View changes

highfive assigned jdm Oct 29, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Oct 31, 2016

shravan-achar force-pushed the shravan-achar:master branch from 010e606 to c75ff9c Oct 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 31, 2016

shravan-achar force-pushed the shravan-achar:master branch from c75ff9c to d45eeac Oct 31, 2016

jdm requested changes Nov 1, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 1, 2016

highfive added the S-needs-rebase label Nov 3, 2016

shravan-achar force-pushed the shravan-achar:master branch from d45eeac to 63f9610 Nov 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 4, 2016

jdm added S-needs-code-changes and removed S-needs-rebase S-awaiting-review labels Nov 4, 2016

jdm added the S-fails-tidy label Nov 4, 2016

shravan-achar force-pushed the shravan-achar:master branch from 63f9610 to 3fbf458 Nov 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 5, 2016

jdm removed the S-fails-tidy label Nov 7, 2016

HTML image map support

Loading status checks…

980dde7

Manishearth force-pushed the shravan-achar:master branch from 0b18af2 to 980dde7 Jan 11, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 11, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 11, 2017

jdm added S-needs-code-changes and removed S-tests-failed labels Jan 11, 2017

Make area structs accessible from area tests

Loading status checks…

d3be70b

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 11, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 11, 2017

bors-servo merged commit d3be70b into servo:master Jan 11, 2017
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 Jan 11, 2017

bors-servo mentioned this pull request Jan 11, 2017

Remove network requests from image cache thread #14962

Merged

4 of 4 tasks complete

servo / servo

Join GitHub today

ImageMaps: Implemented support for parsing coord attribute to a shape… #13972

ImageMaps: Implemented support for parsing coord attribute to a shape… #13972

Conversation

shravan-achar commented Oct 29, 2016 • edited by larsbergstrom

emilio 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Oct 29, 2016

shravan-achar commented Oct 31, 2016

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shravan-achar Nov 2, 2016 • edited

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.

This comment has been minimized.

bors-servo commented Nov 3, 2016

jdm commented Nov 4, 2016

shravan-achar commented Nov 5, 2016 • edited

shravan-achar commented Jan 11, 2017

Manishearth commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

Manishearth commented Jan 11, 2017

emilio commented Jan 11, 2017

jdm commented Jan 11, 2017

Manishearth commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

shravan-achar commented Oct 29, 2016 •

edited by larsbergstrom

shravan-achar Nov 2, 2016 •

edited

shravan-achar commented Nov 5, 2016 •

edited