mozbrowsericonchange event (Browser API)

[paulrouget]

fixes #8347

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 4 of 4 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 129 [r1] (raw file):}
We should use attr.value() instead of href.

---

^{components/script/dom/htmllinkelement.rs, line 133 [r1] (raw file):}
Should this be sizes?

---

^{components/script/dom/htmllinkelement.rs, line 136 [r1] (raw file):}
This can be if let.

---

^{components/script/dom/htmllinkelement.rs, line 139 [r1] (raw file):}
Let's use attr :)

---

^{components/script/dom/htmllinkelement.rs, line 239 [r1] (raw file):}
These should be &str instead of &String; callers will probably need &*.

---

^{components/script/dom/htmllinkelement.rs, line 248 [r1] (raw file):}
Please replace this with a call to document.trigger_mozbrowser_event.

---

^{tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsericonchange_event.html, line 24 [r2] (raw file):}
Use t.step_func(function ...).

---

^{tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsericonchange_event.html, line 34 [r2] (raw file):}
I don't understand this - don't we want to test that the events appear in the order we expect?

---

Comments from the review on Reviewable.io

[jdm]

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:128:58: 128:72 error: Unknown static atom sizes

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:128                     let sizes = get_attr(self.upcast(), &atom!("sizes"));

                                                                                                                                     ^~~~~~~~~~~~~~

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:138:66: 138:80 error: Unknown static atom sizes

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:138                             let sizes = get_attr(self.upcast(), &atom!("sizes"));

                                                                                                                                             ^~~~~~~~~~~~~~

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:172:44: 172:58 error: Unknown static atom sizes

/home/travis/build/servo/servo/components/script/dom/htmllinkelement.rs:172             let sizes = get_attr(element, &atom!("sizes"));

                                                                                                                       ^~~~~~~~~~~~~~

error: aborting due to 3 previous errors

We can use Atom::from_slice("sizes") instead of the atom! macro.

[Ms2ger]

And you should add sizes to https://github.com/servo/string-cache/blob/master/shared/static_atom_list.rs. Pulling that change into Servo is a bunch of work, so feel free to file a followup to actually use the static atom here, rather than blocking this PR on it.

[paulrouget]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 136 [r1] (raw file):}
how so? by checking is_some() later and unwrap?

---

Comments from the review on Reviewable.io

[paulrouget]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 136 [r1] (raw file):}
Oh, I think I got it. if let Some() = …. This will work.

---

Comments from the review on Reviewable.io

[paulrouget]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 239 [r1] (raw file):}
get_attr returns a Option<String>. MozBrowserEvent are made of String (looking at MozBrowserEvent::TitleChange and MozBrowserEvent::LocationChange) and eventually converted to DOMString. Can you explain when str should be used and why it should be used here?

---

Comments from the review on Reviewable.io

[jdm]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 239 [r1] (raw file):}
&String is the useless cousin of &str, since it requires an extra pointer dereference to reach the actual string value and doesn't provide any different interface since it's not mutable. &*some_String_value yields &str due to the Deref implementation for String. String means "I own this string's data", &str means "I am borrowing this string's data", and "&String" means "I am borrowing this string's data inefficiently".

---

Comments from the review on Reviewable.io

[jdm]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmllinkelement.rs, line 239 [r1] (raw file):}
It's the same as vectors - Vec<T> is ownership, &[T] is a slice, and &Vec<T> is pointless, since it's partway to a slice and can't be mutated.

---

Comments from the review on Reviewable.io

[paulrouget]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

^{tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsericonchange_event.html, line 34 [r2] (raw file):}
You're right.

---

Comments from the review on Reviewable.io

[paulrouget]

servo/string-cache#120

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[jdm]

@bors-servo: r+

---

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit c4d5a6a has been approved by jdm

[jdm]

Thanks for doing this work!

[bors-servo]

⌛ Testing commit c4d5a6a with merge bc6991f...

[bors-servo]

💔 Test failed - linux-rel

[larsbergstrom]

@paulrouget My apologies for breaking the builder and leaving you stuck with another rebase after other stuff landed ahead of you!

[jdm]

@bors-servo: delegate+

[bors-servo]

✌️ @paulrouget can now approve this pull request

[paulrouget]

htmliframeelement.rs:147:34: 147:43 error: cannot invoke tuple struct constructor with private fields [E0450]
htmliframeelement.rs:147                 let event_name = DOMString(event.name().to_owned());
                                                          ^~~~~~~~~
htmliframeelement.rs:248:31: 248:40 error: cannot invoke tuple struct constructor with private fields [E0450]
htmliframeelement.rs:248                     rel: Some(DOMString(rel)),
                                                       ^~~~~~~~~
htmliframeelement.rs:249:32: 249:41 error: cannot invoke tuple struct constructor with private fields [E0450]
htmliframeelement.rs:249                     href: Some(DOMString(href)),
                                                        ^~~~~~~~~
htmliframeelement.rs:250:33: 250:42 error: cannot invoke tuple struct constructor with private fields [E0450]
htmliframeelement.rs:250                     sizes: Some(DOMString(sizes)),

[paulrouget]

I see: #8477

[jdm]

@bors-servo: r+

---

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 5263a4c has been approved by jdm

[bors-servo]

⌛ Testing commit 5263a4c with merge 419b1f2...

[bors-servo]

💔 Test failed - linux-rel

[jdm]

@bors-servo: retry

• #8487

[bors-servo]

⌛ Testing commit 5263a4c with merge 7f076c6...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt