Fix Issue 12343: Add pref-gated methods and test for activatable element #12703

sjmelia · 2016-08-02T23:18:39Z

Add methods to activatable elements; gated by a preference, for use in servo-specific WPT tests.

I cargo-culted over the "redundant check" from #12322, I don't really understand the possible exploit.

Obvious flaw is the copypasta for each element. I understand that the webidl codegen will produce a trait for ActivatableElement, but I can't see how I can provide a default implementation for this? I guess otherwise could be an improvement to have each element just delegate to an Enter.../Exit... method on the Activatable trait?

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

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

…states

This change is

izgzhen · 2016-08-03T09:37:30Z

components/script/dom/webidls/ActivatableElement.webidl

+[NoInterfaceObject]
+interface ActivatableElement {
+  [Pref="dom.testing.element.activation.enabled"]
+  void enter_formal_activation_state();


I suppose enterFormalActivationState conforms to the convention better?

izgzhen · 2016-08-03T09:41:08Z

I think the check is redundant here since the code just lies in the script side.

But in the file manager case, the code runs in another process (i.e. net), thus whether the script side interface code is generated or not, the file manager interface will always be the same. net has to check pref again to confirm the status of script side.

izgzhen · 2016-08-03T09:45:00Z

In case you are not aware, you can use ./mach update-manifest instead of manually modify the MANIFEST.json to fix the CI failure

@jdm I am not sure if we should run the manifest_changed.sh earlier, I mean, like ./mach test-tidy

jdm · 2016-08-03T12:42:29Z

@izgzhen I don't think it's worth it, since it's not a quick operation like test-tidy.

jdm · 2016-08-03T12:42:40Z

r? @Manishearth

Manishearth · 2016-08-03T16:14:53Z

Use a macro (dom/macros.rs) to get rid of the copypasta. This will make it easy to add more pref-gated activation methods in the future too -- I'll want to add more invasive tests for general activation behavior at some point which may need more hooks.

Once the macro change and Zhen's comment are addressed, r=me

jdm · 2016-08-03T18:10:11Z

One suggestion I have - what if the ActivatableElement mixin only applied to Element, since the implementation of the methods calls as_element() anyways?

Manishearth · 2016-08-03T18:34:36Z

Oh, that works. Not so well for future activation tests, but I can probably manage with the existing methods

sjmelia · 2016-08-07T22:01:14Z

I have fixed the naming convention; removed the redundant check; ran manifest-update.

R.e. "ActivatableElement mixin only applied to Element" i've interpreted this as meaning to have element implement the webidl interface (instead of each activatable type) and throw in JS if it's not applicable.

r? @Manishearth

Manishearth · 2016-08-08T03:53:18Z

@bors-servo r+

thanks!

bors-servo · 2016-08-08T03:53:19Z

📌 Commit 35ad586 has been approved by Manishearth

bors-servo · 2016-08-08T04:50:24Z

⌛ Testing commit 35ad586 with merge c794721...


        Auto merge of #12703 - sjmelia:12343_test_activation, r=Manishearth

Fix Issue 12343: Add pref-gated methods and test for activatable element  Add methods to activatable elements; gated by a preference, for use in servo-specific WPT tests. I cargo-culted over the "redundant check" from #12322, I don't really understand the possible exploit. Obvious flaw is the copypasta for each element. I understand that the webidl codegen will produce a trait for ActivatableElement, but I can't see how I can provide a default implementation for this? I guess otherwise could be an improvement to have each element just delegate to an Enter.../Exit... method on the Activatable trait? ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12343 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____  …states  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12703)

bors-servo · 2016-08-08T05:39:04Z

💔 Test failed - linux-rel

sjmelia · 2016-08-08T07:56:28Z

😊 sorry for wasting time, I think I got distracted updating the manifest and forgot to actually make the test pass...

jdm · 2016-08-08T13:37:34Z

Looks like test rather than async_test would be appropriate for all of them.


        Issue 12343: Add pref-gated methods and test for activatable element …

…states

jdm · 2016-09-17T01:34:23Z

@bors-servo: r=Manishearth

bors-servo · 2016-09-17T01:34:24Z

📌 Commit a9de84a has been approved by Manishearth

bors-servo · 2016-09-17T01:34:26Z

⌛ Testing commit a9de84a with merge 2fb4dd9...


        Auto merge of #12703 - sjmelia:12343_test_activation, r=Manishearth

Fix Issue 12343: Add pref-gated methods and test for activatable element  Add methods to activatable elements; gated by a preference, for use in servo-specific WPT tests. I cargo-culted over the "redundant check" from #12322, I don't really understand the possible exploit. Obvious flaw is the copypasta for each element. I understand that the webidl codegen will produce a trait for ActivatableElement, but I can't see how I can provide a default implementation for this? I guess otherwise could be an improvement to have each element just delegate to an Enter.../Exit... method on the Activatable trait? ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12343 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____  …states  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12703)

jdm · 2016-09-17T01:34:30Z

Sorry this got neglected!

bors-servo · 2016-09-17T02:25:08Z

💔 Test failed - linux-rel

jdm · 2016-09-17T03:24:55Z

@bors-servo: retry
#13303

bors-servo · 2016-09-17T03:24:57Z

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev are reusable. Rebuilding only linux-rel...

bors-servo · 2016-09-17T04:14:12Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

sjmelia force-pushed the sjmelia:12343_test_activation branch from dc6605f to bdf18df Aug 3, 2016

highfive added the S-awaiting-review label Aug 3, 2016

izgzhen reviewed Aug 3, 2016
View changes

izgzhen changed the title ~~Issue 12343: Add pref-gated methods and test for activatable element …~~ Fix Issue 12343: Add pref-gated methods and test for activatable element Aug 3, 2016

highfive assigned Manishearth Aug 3, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 3, 2016

sjmelia force-pushed the sjmelia:12343_test_activation branch from bdf18df to 35ad586 Aug 7, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 8, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 8, 2016

sjmelia force-pushed the sjmelia:12343_test_activation branch from 35ad586 to 12de67c Aug 8, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Aug 8, 2016

Issue 12343: Add pref-gated methods and test for activatable element …

Loading status checks…

a9de84a

…states

sjmelia force-pushed the sjmelia:12343_test_activation branch from 12de67c to a9de84a Aug 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 17, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 17, 2016

bors-servo merged commit a9de84a into servo:master Sep 17, 2016
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

servo / servo

Fix Issue 12343: Add pref-gated methods and test for activatable element #12703

Fix Issue 12343: Add pref-gated methods and test for activatable element #12703

sjmelia commented Aug 2, 2016 •

edited

This comment has been minimized.

izgzhen commented Aug 3, 2016

izgzhen commented Aug 3, 2016 •

edited

jdm commented Aug 3, 2016

jdm commented Aug 3, 2016

Manishearth commented Aug 3, 2016 •

edited

jdm commented Aug 3, 2016

Manishearth commented Aug 3, 2016

sjmelia commented Aug 7, 2016

Manishearth commented Aug 8, 2016

bors-servo commented Aug 8, 2016

bors-servo commented Aug 8, 2016

bors-servo commented Aug 8, 2016

sjmelia commented Aug 8, 2016

jdm commented Aug 8, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

servo / servo

Join GitHub today

Fix Issue 12343: Add pref-gated methods and test for activatable element #12703

Fix Issue 12343: Add pref-gated methods and test for activatable element #12703

Conversation

sjmelia commented Aug 2, 2016 • edited

This comment has been minimized.

izgzhen Aug 3, 2016

izgzhen commented Aug 3, 2016

izgzhen commented Aug 3, 2016 • edited

jdm commented Aug 3, 2016

jdm commented Aug 3, 2016

Manishearth commented Aug 3, 2016 • edited

jdm commented Aug 3, 2016

Manishearth commented Aug 3, 2016

sjmelia commented Aug 7, 2016

Manishearth commented Aug 8, 2016

bors-servo commented Aug 8, 2016

bors-servo commented Aug 8, 2016

bors-servo commented Aug 8, 2016

sjmelia commented Aug 8, 2016

jdm commented Aug 8, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

sjmelia commented Aug 2, 2016 •

edited

izgzhen commented Aug 3, 2016 •

edited

Manishearth commented Aug 3, 2016 •

edited