Audit usages of unicode case-changing methods. #17883

frewsxcv · 2017-07-27T00:13:36Z

Part of #17777.

This change is


        Fix typo in comment.


        Audit usages of unicode case-changing methods.

highfive · 2017-07-27T00:13:40Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/script/lib.rs, components/script/dom/htmlelement.rs, components/script/dom/htmlareaelement.rs, components/script/dom/blob.rs
@KiChjang: components/script/dom/document.rs, components/net/fetch/methods.rs, components/script/lib.rs, components/script/dom/htmlelement.rs, components/script/dom/htmlareaelement.rs and 1 more

highfive · 2017-07-27T00:13:41Z

Warning

These commits modify net and script code, but no tests are modified. Please consider adding a test!

frewsxcv · 2017-07-27T00:14:50Z

r? @SimonSapin

N.B.: I didn't address all the instances of to_uppercase, to_lowercase, etc.

frewsxcv · 2017-07-27T00:23:39Z

components/script/dom/document.rs

-        "origin-when-cross-origin" => Some(ReferrerPolicy::OriginWhenCrossOrigin),
-        "always" | "unsafe-url" => Some(ReferrerPolicy::UnsafeUrl),
-        "" => Some(ReferrerPolicy::NoReferrer),
+    match token {


should https://docs.rs/cssparser/0.18.2/cssparser/macro.match_ignore_ascii_case.html be used here?

SimonSapin · 2017-07-27T00:24:07Z

components/script/dom/document.rs

-        "origin-when-cross-origin" => Some(ReferrerPolicy::OriginWhenCrossOrigin),
-        "always" | "unsafe-url" => Some(ReferrerPolicy::UnsafeUrl),
-        "" => Some(ReferrerPolicy::NoReferrer),
+    match token {


We have a match_ignore_ascii_case! macro that does this more efficiently.

Addressed in 719b09c

SimonSapin · 2017-07-27T00:24:55Z

components/script/dom/htmlareaelement.rs

-           "rect" => Shape::Rectangle,
-           "polygon" => Shape::Rectangle,
-           "poly" => Shape::Polygon,
+       let shp: Shape = match &shape {


match_ignore_ascii_case! again

Addressed in 719b09c

frewsxcv · 2017-07-27T10:17:23Z

On a side note, that macro seems like a pretty helpful utility. Wonder if it's deserving of its own separate crate instead of (awkwardly) in the cssparser crate

SimonSapin · 2017-07-27T15:18:00Z

Thanks!

@bors-servo r+

Regarding the macro’s location, the thing is we have many such utilities and I don’t feel like dealing with the maintenance overhead of publishing many crates (especially if they’d be in many repositories).

bors-servo · 2017-07-27T15:18:03Z

📌 Commit 719b09c has been approved by SimonSapin

bors-servo · 2017-07-27T15:18:09Z

⌛ Testing commit 719b09c with merge 0de3e03...


        Auto merge of #17883 - frewsxcv:frewsxcxv-lowercase, r=SimonSapin

Audit usages of unicode case-changing methods. Part of #17777.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17883)

bors-servo · 2017-07-27T15:49:08Z

💔 Test failed - linux-rel-wpt

bors-servo · 2017-07-28T09:14:07Z

⌛ Testing commit 719b09c with merge 039810d...


        Auto merge of #17883 - frewsxcv:frewsxcxv-lowercase, r=SimonSapin

Audit usages of unicode case-changing methods. Part of #17777.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17883)

bors-servo · 2017-07-28T09:50:19Z

💔 Test failed - linux-rel-wpt

frewsxcv · 2017-07-28T12:50:27Z

Which tests are failing here?

SimonSapin · 2017-07-28T13:01:15Z

The logs are verbose, so I scripted a thing:

% curl -s http://build.servo.org/builders/linux-rel-wpt/builds/5243/steps/shell__2/logs/filtered-wpt-errorsummary.log/text | jq '{status: .status, test: .test}' -c|sort -u

{"status":"CRASH","test":"/html/dom/elements/global-attributes/dataset-enumeration.html"}
{"status":"FAIL","test":"/custom-elements/reactions/DOMStringMap.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/custom-attrs.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/dataset-delete.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/dataset-get.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/dataset.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/dataset-set.html"}
{"status":"FAIL","test":"/html/dom/elements/global-attributes/data_unicode_attr.html"}
{"status":"FAIL","test":"/html/semantics/embedded-content/the-img-element/current-pixel-density/basic.html"}
{"status":"FAIL","test":"/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html"}
{"status":"FAIL","test":"/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html"}
{"status":"FAIL","test":"/html/semantics/embedded-content/the-img-element/update-the-source-set.html"}
{"status":"TIMEOUT","test":"/dom/collections/domstringmap-supported-property-names.html"}
{"status":"TIMEOUT","test":"/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html"}

frewsxcv · 2017-07-28T18:35:45Z

Latest force push has this change:

diff --git a/components/script/dom/htmlelement.rs b/components/script/dom/htmlelement.rs
index 05fcfcc8ba..1c1f8b8cbf 100644
--- a/components/script/dom/htmlelement.rs
+++ b/components/script/dom/htmlelement.rs
@@ -378,6 +378,7 @@ static DATA_HYPHEN_SEPARATOR: char = '\x2d';
 
 fn to_snake_case(name: DOMString) -> DOMString {
     let mut attr_name = String::with_capacity(name.len() + DATA_PREFIX.len());
+    attr_name.push_str(DATA_PREFIX);
     for ch in name.chars() {
         if ch.is_ascii_uppercase() {
             attr_name.push(DATA_HYPHEN_SEPARATOR);

@bors-servo try

bors-servo · 2017-07-28T18:35:50Z

⌛ Trying commit cc6ba49 with merge 93ac7db...


        Auto merge of #17883 - frewsxcv:frewsxcxv-lowercase, r=<try>

Audit usages of unicode case-changing methods. Part of #17777.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17883)

bors-servo · 2017-07-28T19:30:49Z

💔 Test failed - linux-rel-wpt

jdm · 2017-07-28T21:30:22Z

▶ CRASH [expected OK] /html/dom/elements/global-attributes/dataset-enumeration.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ attempt to subtract with overflow (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at /home/servo/buildbot/slave/linux-rel-wpt/build/components/script/dom/htmlelement.rs:408)

  ▶ TIMEOUT [expected OK] /dom/collections/domstringmap-supported-property-names.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  └ attempt to subtract with overflow (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at /home/servo/buildbot/slave/linux-rel-wpt/build/components/script/dom/htmlelement.rs:408)


        Minor refactorings for 'data-' attribute logic.


        Utilize match_ignore_ascii_case! in more places.

frewsxcv · 2017-07-29T09:58:35Z

Latest force push:

diff --git a/components/script/dom/htmlelement.rs b/components/script/dom/htmlelement.rs
index 1c1f8b8cbf..05683653fa 100644
--- a/components/script/dom/htmlelement.rs
+++ b/components/script/dom/htmlelement.rs
@@ -405,7 +405,7 @@ fn to_camel_case(name: &str) -> Option<DOMString> {
     if has_uppercase {
         return None;
     }
-    let mut result = String::with_capacity(name.len() - DATA_PREFIX.len());
+    let mut result = String::with_capacity(name.len().saturating_sub(DATA_PREFIX.len()));
     let mut name_chars = name.chars();
     while let Some(curr_char) = name_chars.next() {
         //check for hyphen followed by character

@bors-servo try

bors-servo · 2017-07-29T09:58:39Z

⌛ Trying commit befe538 with merge bc4028c...


        Auto merge of #17883 - frewsxcv:frewsxcxv-lowercase, r=<try>

Audit usages of unicode case-changing methods. Part of #17777.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17883)

bors-servo · 2017-07-29T11:01:34Z

☀️ 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
State: approved= try=True

frewsxcv · 2017-07-29T11:02:28Z

r? @SimonSapin

SimonSapin · 2017-08-02T07:49:10Z

@bors-servo r+

Thanks!

Reviewed 1 of 1 files at r1, 3 of 6 files at r2, 1 of 1 files at r4, 5 of 5 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2017-08-02T07:49:10Z

📌 Commit befe538 has been approved by SimonSapin

bors-servo · 2017-08-02T08:01:29Z

⌛ Testing commit befe538 with merge 5fff90c...


        Auto merge of #17883 - frewsxcv:frewsxcxv-lowercase, r=SimonSapin

Audit usages of unicode case-changing methods. Part of #17777.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17883)

bors-servo · 2017-08-02T09:10:04Z

☀️ 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: SimonSapin
Pushing 5fff90c to master...

frewsxcv added 2 commits Jul 26, 2017

Fix typo in comment.

58fd295

Audit usages of unicode case-changing methods.

23e5bfa

highfive assigned glennw Jul 27, 2017

highfive added the S-awaiting-review label Jul 27, 2017

highfive assigned SimonSapin and unassigned glennw Jul 27, 2017

frewsxcv reviewed Jul 27, 2017

View changes

SimonSapin reviewed Jul 27, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 27, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 27, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 28, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 28, 2017

frewsxcv force-pushed the frewsxcv:frewsxcxv-lowercase branch from 719b09c to cc6ba49 Jul 28, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jul 28, 2017

highfive added the S-tests-failed label Jul 28, 2017

frewsxcv added 2 commits Jul 27, 2017

Minor refactorings for 'data-' attribute logic.

6238035

Utilize match_ignore_ascii_case! in more places.

Loading status checks…

befe538

frewsxcv force-pushed the frewsxcv:frewsxcxv-lowercase branch from cc6ba49 to befe538 Jul 29, 2017

highfive removed the S-tests-failed label Jul 29, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 2, 2017

bors-servo merged commit befe538 into servo:master Aug 2, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 2, 2017

frewsxcv deleted the frewsxcv:frewsxcxv-lowercase branch Aug 15, 2017

servo / servo

Audit usages of unicode case-changing methods. #17883

Audit usages of unicode case-changing methods. #17883

frewsxcv commented Jul 27, 2017 •

edited by larsbergstrom

highfive commented Jul 27, 2017

highfive commented Jul 27, 2017

frewsxcv commented Jul 27, 2017

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.

frewsxcv commented Jul 27, 2017

SimonSapin commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

frewsxcv commented Jul 28, 2017

SimonSapin commented Jul 28, 2017 •

edited

frewsxcv commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

jdm commented Jul 28, 2017

frewsxcv commented Jul 29, 2017

bors-servo commented Jul 29, 2017

bors-servo commented Jul 29, 2017

frewsxcv commented Jul 29, 2017

SimonSapin commented Aug 2, 2017

bors-servo commented Aug 2, 2017

bors-servo commented Aug 2, 2017

bors-servo commented Aug 2, 2017

servo / servo

Join GitHub today

Audit usages of unicode case-changing methods. #17883

Audit usages of unicode case-changing methods. #17883

Conversation

frewsxcv commented Jul 27, 2017 • edited by larsbergstrom

highfive commented Jul 27, 2017

highfive commented Jul 27, 2017

frewsxcv commented Jul 27, 2017

This comment has been minimized.

frewsxcv Jul 27, 2017

This comment has been minimized.

SimonSapin Jul 27, 2017

This comment has been minimized.

frewsxcv Jul 27, 2017

This comment has been minimized.

SimonSapin Jul 27, 2017

This comment has been minimized.

frewsxcv Jul 27, 2017

frewsxcv commented Jul 27, 2017

SimonSapin commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

frewsxcv commented Jul 28, 2017

SimonSapin commented Jul 28, 2017 • edited

frewsxcv commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

jdm commented Jul 28, 2017

frewsxcv commented Jul 29, 2017

bors-servo commented Jul 29, 2017

bors-servo commented Jul 29, 2017

frewsxcv commented Jul 29, 2017

SimonSapin commented Aug 2, 2017

bors-servo commented Aug 2, 2017

bors-servo commented Aug 2, 2017

bors-servo commented Aug 2, 2017

frewsxcv commented Jul 27, 2017 •

edited by larsbergstrom

SimonSapin commented Jul 28, 2017 •

edited