Skip to content
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

Implement Ellipse Canvas 2D API #18242

Merged
merged 1 commit into from Sep 2, 2017
Merged

Implement Ellipse Canvas 2D API #18242

merged 1 commit into from Sep 2, 2017

Conversation

@joone
Copy link
Contributor

joone commented Aug 25, 2017

This patch needs to update rust-azure to 0.21.0.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17598
  • [] There are tests for these changes OR
  • [] These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Aug 25, 2017

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

@highfive

This comment has been minimized.

Copy link

highfive commented Aug 25, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/webidls/CanvasRenderingContext2D.webidl
  • @KiChjang: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/webidls/CanvasRenderingContext2D.webidl
@highfive

This comment has been minimized.

Copy link

highfive commented Aug 25, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 25, 2017

Is it okay to add a test case: tests/wpt/web-platform-tests/offscreen-canvas/path-objects/2d.path.ellipse.html?

@stshine

This comment has been minimized.

Copy link
Contributor

stshine commented Aug 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 26, 2017

⌛️ Trying commit 64c727b with merge 9a1019d...

bors-servo added a commit that referenced this pull request Aug 26, 2017
Implement Ellipse Canvas 2D API

This patch needs to update rust-azure to 0.21.0.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17598

<!-- Either: -->
- [] There are tests for these changes OR
- [] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18242)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 26, 2017

💔 Test failed - windows-msvc-dev

@stshine

This comment has been minimized.

Copy link
Contributor

stshine commented Aug 27, 2017


"C:\buildbot\slave\windows-msvc-dev\build\target\debug\build\azure-3f01c03fa9e237b5\out\build\install.vcxproj" (default target) (1) ->
"C:\buildbot\slave\windows-msvc-dev\build\target\debug\build\azure-3f01c03fa9e237b5\out\build\ALL_BUILD.vcxproj" (default target) (3) ->
"C:\buildbot\slave\windows-msvc-dev\build\target\debug\build\azure-3f01c03fa9e237b5\out\build\azure.vcxproj" (default target) (4) ->
(ClCompile target) -> 
  C:\Users\Administrator\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\DrawTargetD2D.cpp(1294): error C2259: 'mozilla::gfx::PathBuilderD2D': cannot instantiate abstract class [C:\buildbot\slave\windows-msvc-dev\build\target\debug\build\azure-3f01c03fa9e237b5\out\build\azure.vcxproj]
  C:\Users\Administrator\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\PathD2D.cpp(348): error C2259: 'mozilla::gfx::PathBuilderD2D': cannot instantiate abstract class [C:\buildbot\slave\windows-msvc-dev\build\target\debug\build\azure-3f01c03fa9e237b5\out\build\azure.vcxproj]
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 27, 2017

I'm fixing the build break: servo/rust-azure#273

@joone joone force-pushed the joone:ellipse branch from 64c727b to 7ea84ff Aug 28, 2017
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 28, 2017

@stshine My fix landed in rust-azure. Can you run build bots again?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 28, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 28, 2017

⌛️ Trying commit 7ea84ff with merge bdab299...

bors-servo added a commit that referenced this pull request Aug 28, 2017
Implement Ellipse Canvas 2D API

This patch needs to update rust-azure to 0.21.0.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17598

<!-- Either: -->
- [] There are tests for these changes OR
- [] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18242)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 28, 2017

This doesn't look good:

C:\projects\servo\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\DrawTargetD2D.cpp(1294): error C2259: 'mozilla::gfx::PathBuilderD2D': cannot instantiate abstract class [C:\projects\servo\target\debug\build\azure-3f01c03fa9e237b5\out\build\azure.vcxproj]
  C:\projects\servo\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\DrawTargetD2D.cpp(1294): note: due to following members:
  C:\projects\servo\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\DrawTargetD2D.cpp(1294): note: 'void mozilla::gfx::PathSink::Arc(const mozilla::gfx::Point &,float,float,float,float,float,bool)': is abstract
  c:\projects\servo\.cargo\git\checkouts\rust-azure-984e17605d95e219\a2351f2\libazure\2D.h(451): note: see declaration of 'mozilla::gfx::PathSink::Arc'
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 28, 2017

💔 Test failed - windows-msvc-dev

@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 30, 2017

@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 30, 2017

@jdm Can you run build bots?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 30, 2017

I think you need to run ./mach cargo-update -p azure and push the changes before they will show up.

@joone joone force-pushed the joone:ellipse branch from 7ea84ff to dc4b6d0 Aug 30, 2017
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Aug 30, 2017

@jdm Done, Thanks!

@stshine

This comment has been minimized.

Copy link
Contributor

stshine commented Aug 30, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 30, 2017

⌛️ Trying commit dc4b6d0 with merge 5bb142d...

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Sep 1, 2017

@bors-servo r+
Thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 1, 2017

📌 Commit fd09294 has been approved by jdm

@highfive highfive assigned jdm and unassigned emilio Sep 1, 2017
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 1, 2017

⌛️ Testing commit fd09294 with merge d4b20df...

bors-servo added a commit that referenced this pull request Sep 1, 2017
Implement Ellipse Canvas 2D API

This patch needs to update rust-azure to 0.21.0.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17598

<!-- Either: -->
- [] There are tests for these changes OR
- [] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18242)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 1, 2017

💔 Test failed - linux-rel-wpt

@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Sep 1, 2017

@jdm Here is a log from linux-rel-wpt:
{"status": "FAIL", "group": "default", "message": "/_mozilla/mozilla/css-paint-api/background-image-tiled.html ebc42c91261f94d0907b46e396e609ba822678cb\n/_mozilla/mozilla/css-paint-api/background-image-tiled-ref.html 35ba3945e6b317536fde7758020490913812c005\nTesting ebc42c91261f94d0907b46e396e609ba822678cb == 35ba3945e6b317536fde7758020490913812c005", "stack": null, "subtest": null, "test": "/_mozilla/mozilla/css-paint-api/background-image-tiled.html", "line": 93729, "action": "test_result", "expected": "PASS"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "CanvasRenderingContext2D interface: document.createElement("canvas").getContext("2d") must inherit property "ellipse" with the proper type (78)", "test": "/html/dom/interfaces.html", "line": 221873, "action": "test_result", "expected": "FAIL"}

Hmm, I fixed this problem by adding the ini file.

@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Sep 1, 2017

@jdm Do you mean that we don't need the new ini file and we have to remove the following line from interface.ini file?
[CanvasRenderingContext2D interface: document.createElement("canvas").getContext("2d") must inherit property "ellipse" with the proper type (78)]
expected: FAIL

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Sep 1, 2017

We still need the new ini file, but I gave you the wrong directory previously. And yes, we need to remove the lines that I linked from interfaces.html.ini.

* Update rust-azure to 0.21.0
* Mark the following test case as fail:
  tests/wpt/mozilla/tests/mozilla/css-paint-api/background-image-tiled.html
* Make the ellipse test case pass.

BUG: #17598
@joone joone force-pushed the joone:ellipse branch from fd09294 to 57e283a Sep 1, 2017
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Sep 2, 2017

@jdm can you take a look at the error?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Sep 2, 2017

@bors-servo: r+
The travisCI error is #18346.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 2, 2017

📌 Commit 57e283a has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 2, 2017

⌛️ Testing commit 57e283a with merge 3a4b98a...

bors-servo added a commit that referenced this pull request Sep 2, 2017
Implement Ellipse Canvas 2D API

This patch needs to update rust-azure to 0.21.0.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17598

<!-- Either: -->
- [] There are tests for these changes OR
- [] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18242)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 2, 2017

@bors-servo bors-servo merged commit 57e283a into servo:master Sep 2, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@joone

This comment has been minimized.

Copy link
Contributor Author

joone commented Sep 2, 2017

@jdm Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.