New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DOMQuad element #8882

Merged
merged 1 commit into from Dec 18, 2015

Conversation

Projects
None yet
7 participants
@GuillaumeGomez
Contributor

GuillaumeGomez commented Dec 7, 2015

cc @nox

Part of #8821

Review on Reviewable

@nox

This comment has been minimized.

Member

nox commented Dec 10, 2015

./components/script/dom/domquad.rs:21: use statement is not in alphabetical order
    expected: util::str::DOMString
    found: dom::dompoint::DOMPointReadOnly
[Constructor(optional DOMPointInit p1, optional DOMPointInit p2,
optional DOMPointInit p3, optional DOMPointInit p4),
Exposed=Window,Exposed=Worker]

This comment has been minimized.

@nox

nox Dec 10, 2015

Member

This is not the exposed set you are looking for, it should be "Exposed=(Window,Worker)".

@nox

This comment has been minimized.

Member

nox commented Dec 10, 2015

-S-awaiting-review +S-blocked-on-external

This needs the tests in #8915.


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


components/script/dom/dompointreadonly.rs, line 48 [r2] (raw file):
This is not an actual DOM method so it doesn't need the fancy CamelCase name, please rename to new_from_point_init or something like this. Also, this should actually just call new and not new_inherited.


components/script/dom/domquad.rs, line 47 [r2] (raw file):
Please just use the min and max functions directly, with no additional vectors and things like this.


components/script/dom/domquad.rs, line 54 [r2] (raw file):
This should start with a Reflector field.


components/script/dom/domquad.rs, line 59 [r2] (raw file):
All these should be JS<_> fields.


components/script/dom/domquad.rs, line 115 [r2] (raw file):
There is no FromPoint method in your PR.


components/script/dom/domquad.rs, line 121 [r2] (raw file):
These should return Root<DOMPointReadOnly>.


Comments from the review on Reviewable.io

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:dom_quad branch from ba11f92 to 15884f1 Dec 10, 2015

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:dom_quad branch 2 times, most recently from 0170a28 to fbdd3aa Dec 10, 2015

@nox

This comment has been minimized.

Member

nox commented Dec 13, 2015

/home/travis/build/servo/servo/components/script/dom/domquad.rs:15:5: 15:36 error: unresolved import `dom::dompoint::DOMPointReadOnly`. There is no `DOMPointReadOnly` in `dom::dompoint` [E0432]
/home/travis/build/servo/servo/components/script/dom/domquad.rs:15 use dom::dompoint::DOMPointReadOnly;
                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/servo/servo/components/script/dom/domquad.rs:15:5: 15:36 help: run `rustc --explain E0432` to see a detailed explanation
@nox

This comment has been minimized.

Member

nox commented Dec 13, 2015

Reviewed 1 of 2 files at r3, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


components/script/dom/dompointreadonly.rs, line 48 [r5] (raw file):
"dom" not "DOM". Actually just call them new_from_point_init and new_from_point.


components/script/dom/domquad.rs, line 58 [r5] (raw file):
That can't compile. These fields are JS<_> and that method returns Root<_>.


components/script/dom/domquad.rs, line 158 [r5] (raw file):
This is already generated by codegen. If it isn't, you forgot something in one of the WebIDL files.


Comments from the review on Reviewable.io

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:dom_quad branch from fbdd3aa to 804e105 Dec 13, 2015

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:dom_quad branch from 804e105 to a92500d Dec 13, 2015

bors-servo added a commit that referenced this pull request Dec 14, 2015

Auto merge of #8966 - GuillaumeGomez:patch-1, r=nox
Fix invalid dictionary inheritance

Needed by #8882.

cc @nox

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8966)
<!-- Reviewable:end -->
@nox

This comment has been minimized.

Member

nox commented Dec 17, 2015

Thanks for coping with my bad explanations.

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 17, 2015

⌛️ Testing commit 7467770 with merge 7be4496...

bors-servo added a commit that referenced this pull request Dec 17, 2015

Auto merge of #8882 - GuillaumeGomez:dom_quad, r=nox
Add DOMQuad element

cc  @nox

Part of #8821

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8882)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 17, 2015

💔 Test failed - linux-rel

@jdm

This comment has been minimized.

Member

jdm commented Dec 17, 2015

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.html:
  │ FAIL [expected PASS] Interfaces exposed on the window
  │   → assert_true: If this is failing: DANGER, are you sure you want to expose the new interface DOMQuad to all webpages as a property on the window? Do not make a change to this file without review from jdm or Ms2ger for that specific change! expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:249:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:243:1
@jdm

This comment has been minimized.

Member

jdm commented Dec 17, 2015

We'll need to add the DOMQuad interface to the list in mozilla/interfaces.html.

@jdm

This comment has been minimized.

Member

jdm commented Dec 17, 2015

The other failure is #9012.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:dom_quad branch from 7467770 to 823e1b9 Dec 18, 2015

@nox

This comment has been minimized.

Member

nox commented Dec 18, 2015

@bors-servo r+


Reviewed 1 of 1 files at r18.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 18, 2015

📌 Commit 823e1b9 has been approved by nox

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 18, 2015

⌛️ Testing commit 823e1b9 with merge 6ba4ef2...

bors-servo added a commit that referenced this pull request Dec 18, 2015

Auto merge of #8882 - GuillaumeGomez:dom_quad, r=nox
Add DOMQuad element

cc  @nox

Part of #8821

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8882)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 18, 2015

💔 Test failed - mac-rel-wpt

@jdm

This comment has been minimized.

Member

jdm commented Dec 18, 2015

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 18, 2015

⚡️ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 18, 2015

@bors-servo bors-servo merged commit 823e1b9 into servo:master Dec 18, 2015

3 checks passed

code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:dom_quad branch Jan 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment