Added implementation for itemprop and itemtype along with test files #19038

CJ8664 · 2017-10-27T10:31:10Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR

This change is

highfive · 2017-10-27T10:31:16Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

highfive · 2017-10-27T10:31:17Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/htmlelement.rs
@KiChjang: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/htmlelement.rs

CJ8664 · 2017-10-27T13:52:24Z

@KiChjang , @fitzgen , @jdm
The build error says

error: no rules expected the token "itemtype"--> C:\projects\servo\components\script\dom\htmlelement.rs:316:26 | 316 | &local_name!("itemtype"), | ^^^^^^^^^^ error: no rules expected the token"itemprop"--> C:\projects\servo\components\script\dom\htmlelement.rs:346:26 | 346 | &local_name!("itemprop"), | ^^^^^^^^^^ error: no rules expected the token"itemprop"--> C:\projects\servo\components\script\dom\htmlelement.rs:696:26 | 696 | &local_name!("itemprop") => AttrValue::from_serialized_tokenlist(value.into()), | ^^^^^^^^^^ error: no rules expected the token"itemtype"--> C:\projects\servo\components\script\dom\htmlelement.rs:697:26 | 697 | &local_name!("itemtype") => AttrValue::from_serialized_tokenlist(value.into()), | ^^^^^^^^^^ error: aborting due to 4 previous errors error: Could not compilescript.

Whereas we had merged a pull request to add these into html5ever https://github.com/servo/html5ever/pull/320/files

KiChjang · 2017-10-27T18:02:44Z

Have you ran ./mach update-cargo -p markup5ever?

Also, merge commits are not accepted. Please rebase against the master branch.

CJ8664 · 2017-10-27T21:03:22Z

Closing pull request to make minor tidy modifications

KiChjang · 2017-10-27T23:16:18Z

tests/wpt/mozilla/meta/MANIFEST.json

+    [
+     {}
+    ]
+   ],


This... doesn't seem it should be here.

KiChjang · 2017-10-27T23:16:40Z

tests/wpt/mozilla/meta/MANIFEST.json

+  "mozilla/.DS_Store": [
+   "df2fbeb1400acda0909a32c1cf6bf492f1121e07",
+   "support"
+  ],


CJ8664 · 2017-10-28T15:20:53Z

@KiChjang we are having some difficulties in rebasing our branch, is there a possibility/option to squash the commits while merging?

KiChjang · 2017-10-28T22:51:06Z

I can help you rebase against master, if you have the "Allow edits from maintainers" checkbox checked at the right hand side of GitHub's interface.

Please let me know whether this is preferable to you, and if you agree to it, please do not push any code to your branch until my rebasing is complete.

KiChjang · 2017-10-28T23:50:16Z

components/script/dom/webidls/HTMLElement.webidl

@@ -17,10 +17,16 @@ interface HTMLElement : Element {
  readonly attribute DOMStringMap dataset;

  // microdata
-  //         attribute boolean itemScope;
+  //           attribute boolean itemScope;
+  //           attribute DOMString itemProp;


Why are these commented out?

I think these lines were already there. We don't need these comments, I can delete them

jdm

Good work!

jdm · 2017-10-30T17:15:49Z

.gitignore

@@ -35,3 +35,5 @@ Servo.app
 .vscode

 /unminified-js
+
+tests/wpt/mozilla/tests/mozilla/microdata/local_tests.sh


I assume this is a script you wrote for your own testing purposes? Let's not add it to .gitignore; you can just leave it as an untracked file.

jdm · 2017-10-30T17:16:25Z

components/script/dom/webidls/HTMLElement.webidl

  //         attribute DOMString itemId;
  //readonly attribute HTMLPropertiesCollection properties;
  //         attribute any itemValue; // acts as DOMString on setting
+  [Pref="dom.microdata.enabled"]


Since these are only for use in test code, let's use the dom.microdata.testing.enabled name instead.

jdm · 2017-10-30T17:16:34Z

resources/prefs.json

@@ -6,6 +6,7 @@
  "dom.customelements.enabled": true,
  "dom.forcetouch.enabled": false,
  "dom.gamepad.enabled": false,
+  "dom.microdata.enabled": true,


dom.microdata.testing.enabled

jdm · 2017-10-30T17:16:44Z

tests/wpt/mozilla/meta/mozilla/microdata/__dir__.ini

@@ -0,0 +1 @@
+prefs: ["dom.microdata.enabled:true"]


dom.microdata.testing.enabled

jdm · 2017-10-30T17:20:27Z

tests/wpt/mozilla/tests/mozilla/microdata/dup_prop_type_test.html

+    // Test body element
+    var body = document.getElementById("body");
+    assert_array_equals(body.propertyNames().sort(), ["hello", "world"].sort(), "The method should return 'hello world' ");
+    assert_array_equals(body.itemtypes().sort(), ["hello", "world"].sort(), "The method should return 'hello world' ");


Let's reduce some of the duplication in this test (and others) like this:

var ids = ["body", "h1", "h2", "p", "input", "form", "li", "ul", "meta", "table", "div", "anchor"]; for (let id of ids) { var elem = document.getElementById(id); assert_array_equals(elem.propertyNames().sort(), ["hello", "world"], "duplicate itemprop tokens should be filtered"); assert_array_equals(elem.itemtypes().sort(), ["hello", "world"], "duplicate itemtype tokens should be filtered"); }

jdm · 2017-10-30T17:23:29Z

components/script/dom/htmlelement.rs

@@ -44,27 +45,30 @@ pub struct HTMLElement {
 }

 impl HTMLElement {
-    pub fn new_inherited(tag_name: LocalName, prefix: Option<Prefix>,
-                         document: &Document) -> HTMLElement {
+    pub fn new_inherited(tag_name: LocalName, prefix: Option<Prefix>, document: &Document) -> HTMLElement {


This file contains a lot of modifications caused by your editor automatically running rustfmt on the file, which makes it harder to review. Could you disable this feature, revert the changes to the file, and make your modifications to it again?

jdm · 2017-10-30T17:25:26Z

components/script/dom/htmlelement.rs

+            self.upcast::<Element>()
+                .GetAttribute(DOMString::from(String::from("itemtype")))
+                .unwrap_or(DOMString::from(String::from(""))),
+        );


Rather than calling parse_plain_attribute here, we should use Element::get_tokenlist_attribute instead, which will return the previously-parsed value.

About this change, do we then remove the parse_plain_attribute method that we implemented ? because its purpose was to be used in this function

No, parse_plain_attribute is a method that gets called from other parts of Servo already. Your implementation is required, like adding a virtual method override in C++.

Got it, we have now used Element::get_tokenlist_attribute and will push the code in few minutes, running test-tidy now. When can we expect the appveyor to work again? ;(

AppVeyor passing is not required for landing. Also, when we finally approve your changes and are ready to land your changes, we have another build & test system that will build and test your changes against all web platform tests before merging your PR to the master branch.

jdm · 2017-10-30T17:28:58Z

components/script/dom/htmlelement.rs

        }
+        let opt: Option<Vec<DOMString>> = Some(item_attr_vector);
+        return opt;


Instead of manually draining and pushing, we can replace this with:

Some(item_attrs.into_iter().collect())

jdm · 2017-10-30T17:29:30Z

components/script/dom/htmlelement.rs

+            self.upcast::<Element>()
+                .GetAttribute(DOMString::from(String::from("itemprop")))
+                .unwrap_or(DOMString::from(String::from(""))),
+        );


Same comments for this method as the previous one.

KiChjang · 2017-10-30T18:08:17Z

@CJ8664 Actually, the better path forward here is to address @jdm's comments first - once we are OK with your changes, we'll rebase this on our side. Sorry for the confusion!

CJ8664 · 2017-10-30T18:45:10Z

Sure @KiChjang , we will incorporate the suggestions given by @jdm and then rebase-> merge. @jdm Thanks for the suggestions, as we are new to Rust, we will keep on improving with further develo[ment and reviews.

bors-servo · 2017-10-31T00:32:59Z

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

vjhebbar · 2017-11-02T02:33:03Z

Thank you for the review comments. We will begin incorporating changes and provide an update.

CJ8664 · 2017-11-02T02:35:37Z

Closing the pull request, to make changes. Will reopen once the changes are pushed

jdm · 2017-11-02T12:12:41Z

It is perfectly acceptable to leave the PR open while you make changes.

KiChjang · 2017-11-06T19:06:43Z

@CJ8664 Not necessary; on the GitHub UI, if you have the "Allow maintainer to edit branch" checkbox checked, then people with write access to the servo repository would be able to push to your PR branch. I've just done that, so you should see all your team's work squashed into 1 commit on this branch.

Thank you for your work!

@bors-servo r=jdm

bors-servo · 2017-11-06T19:06:43Z

📌 Commit 7b3794b has been approved by jdm

bors-servo · 2017-11-06T19:06:48Z

⌛ Testing commit 7b3794b with merge 27056d7...

Added implementation for itemprop and itemtype along with test files  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [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/19038)

bors-servo · 2017-11-06T19:27:30Z

💔 Test failed - android

jdm · 2017-11-06T19:42:37Z

Sigh. There's an extra newline at the end of Cargo.lock that needs to be removed. :(

CJ8664 · 2017-11-06T19:55:44Z

Hey @KiChjang should we remove the extraline from Cargo.lock or pushing our changes onto this PR would mess it up?

jdm · 2017-11-06T20:08:49Z

$ git fetch origin master
$ git checkout master
$ git reset --hard 7b3794b
... make the change ...
$ git commit -a --amend --no-edit
$ git push -f origin master

That should take care of it.

Created test file Added the stub methods for itemprop and itemscope Resolved html5ever dependency, added ItemScope and ItemProp attr Resolving dependency Added pref override on metadata attributes Resetting to original state due to change in requirement Reverted adding attributes 1. add a customized implementation of parse_plain_attribute 2. add the following methods to HTMLElement.webidl added itemprop and itemtype, enabled pref in test Added initial implementation for getting itemprop property values Adding the wireframe for testing Implemented function to handle itemType Corrected typo Fixed typo bug in code Handling duplicates for itemtype attribute values Added the test suite structure Added test for extra space Added test for regular test values Added test cases for Single property values Test cases to check absence of itemtype and itemprop attributes Added code to handle absence of itemtype or itemprop attributes Added shell script to run all test cases cleared up Cargo file Tidying up Removed the local test file Removed new line for test-tidy Ordered key in prefs.json Fixes for test-tidy Enabled test preferences Created test using wpt Creating WPT Tests for Regular and Single Prop Types Fixed the Regular type test Fixed tests Removed old test case metadata Incorporate review changes from PR Updated MANIFEST to sync test cases Making changed suggested in review Removed editor folding Removed unnecessary code Resolving cargo conflicts Updated PropertyNames and itemtypes implementation Trying different data in test case Updated manifest Updated code based on reviews

CJ8664 · 2017-11-06T20:12:50Z

Lets see if it builds this time

jdm · 2017-11-06T20:17:08Z

@bors-servo: r+

bors-servo · 2017-11-06T20:17:08Z

📌 Commit 4a43dba has been approved by jdm

bors-servo · 2017-11-06T20:17:13Z

⌛ Testing commit 4a43dba with merge 5227df2...

Added implementation for itemprop and itemtype along with test files  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [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/19038)

bors-servo · 2017-11-06T21:33:20Z

☀️ Test successful - android, 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 5227df2 to master...

highfive assigned Manishearth Oct 27, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 27, 2017

CJ8664 closed this Oct 27, 2017

CJ8664 reopened this Oct 27, 2017

CJ8664 closed this Oct 27, 2017

KiChjang reviewed Oct 27, 2017

View reviewed changes

tests/wpt/mozilla/meta/MANIFEST.json Outdated

[

{}

]

],

Copy link

Contributor

KiChjang Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This... doesn't seem it should be here.

KiChjang reviewed Oct 27, 2017

View reviewed changes

CJ8664 reopened this Oct 27, 2017

CJ8664 mentioned this pull request Oct 28, 2017

Implement microdata #18528

Open

KiChjang reviewed Oct 28, 2017

View reviewed changes

jdm requested changes Oct 30, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 30, 2017

jdm assigned jdm and unassigned Manishearth Oct 30, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Oct 31, 2017

CJ8664 closed this Nov 2, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 6, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 6, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 6, 2017

CJ8664 force-pushed the master branch from 7b3794b to 4a43dba Compare November 6, 2017 20:12

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 6, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 6, 2017

jdm approved these changes Nov 6, 2017

View reviewed changes

bors-servo merged commit 4a43dba into servo:master Nov 6, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 6, 2017

bors-servo mentioned this pull request Nov 6, 2017

Upgrade to rustc 1.23.0-nightly (02004ef78 2017-11-08) #19128

Merged

Added implementation for itemprop and itemtype along with test files #19038

Added implementation for itemprop and itemtype along with test files #19038

Conversation

CJ8664 commented Oct 27, 2017 • edited

highfive commented Oct 27, 2017

highfive commented Oct 27, 2017

CJ8664 commented Oct 27, 2017 • edited

KiChjang commented Oct 27, 2017

CJ8664 commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CJ8664 commented Oct 28, 2017

KiChjang commented Oct 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang commented Oct 30, 2017

CJ8664 commented Oct 30, 2017

bors-servo commented Oct 31, 2017

vjhebbar commented Nov 2, 2017

CJ8664 commented Nov 2, 2017

jdm commented Nov 2, 2017

KiChjang commented Nov 6, 2017

bors-servo commented Nov 6, 2017

bors-servo commented Nov 6, 2017

bors-servo commented Nov 6, 2017

jdm commented Nov 6, 2017

CJ8664 commented Nov 6, 2017

jdm commented Nov 6, 2017

CJ8664 commented Nov 6, 2017

jdm commented Nov 6, 2017

bors-servo commented Nov 6, 2017

bors-servo commented Nov 6, 2017

bors-servo commented Nov 6, 2017

CJ8664 commented Oct 27, 2017 •

edited

CJ8664 commented Oct 27, 2017 •

edited

KiChjang commented Oct 28, 2017 •

edited