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

OffscreenCanvas API #22495

Merged
merged 1 commit into from Jan 16, 2019

Conversation

Projects
None yet
7 participants
@maharsh312
Copy link
Contributor

maharsh312 commented Dec 19, 2018

Initial Steps for OffscreenCanvas API are done in this commit.

r? @jdm

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14627 (GitHub issue number if applicable)
  • 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 Dec 19, 2018

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

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 19, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/OffscreenCanvasRenderingContext2D.webidl, components/script/dom/mod.rs, components/script/dom/webidls/OffscreenCanvas.webidl, components/script/dom/offscreencanvas.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs
  • @jgraham: tests/wpt/include.ini
  • @KiChjang: components/script/dom/webidls/OffscreenCanvasRenderingContext2D.webidl, components/script/dom/mod.rs, components/script/dom/webidls/OffscreenCanvas.webidl, components/script/dom/offscreencanvas.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Dec 19, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 19, 2018

Please note that ./mach test-tidy is failing, this error is present:

error: unused import: `Error`
 --> components/script/dom/offscreencanvas.rs:9:35
  |
9 | use crate::dom::bindings::error::{Error, Fallible};
  |                                   ^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
@CYBAI

This comment has been minimized.

Copy link
Collaborator

CYBAI commented Dec 20, 2018

error: unreachable pattern
  --> components/script/dom/offscreencanvas.rs:96:17
   |
96 |                 _ => None,
   |                 ^
   |
   = note: `-D unreachable-patterns` implied by `-D warnings`
                                                                                
error: aborting due to previous error
                                                                                
error: Could not compile `script`.
@maharsh312

This comment has been minimized.

Copy link
Contributor Author

maharsh312 commented Dec 23, 2018

@jdm Can you suggest any solution to resolve unreachable pattern warning?

@fabricedesre

This comment has been minimized.

Copy link
Contributor

fabricedesre commented Dec 23, 2018

@jdm Can you suggest any solution to resolve unreachable pattern warning?

There is only one variant in the enum, so you can never match on anything else but OffscreenCanvas2D. For now, just remove the _ => ... part.

@maharsh312

This comment has been minimized.

Copy link
Contributor Author

maharsh312 commented Dec 23, 2018

@jdm Can you suggest any solution to resolve unreachable pattern warning?

There is only one variant in the enum, so you can never match on anything else but OffscreenCanvas2D. For now, just remove the _ => ... part.

Getting this error now: https://pastebin.com/jzTYEkiu

@fabricedesre

This comment has been minimized.

Copy link
Contributor

fabricedesre commented Dec 23, 2018

You removed it from get_or_init_2d_context() right?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Trying commit 6884011 with merge 8ef1072...

bors-servo added a commit that referenced this pull request Jan 9, 2019

Auto merge of #22495 - maharsh312:master, r=<try>
OffscreenCanvas API

<!-- Please describe your changes on the following line: -->
Initial Steps for OffscreenCanvas API are done in this commit.

r? @jdm
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14627 (GitHub issue number if applicable)

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/22495)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

First run:

$ wget https://build.servo.org/builders/linux-rel-css/builds/11406/steps/test/logs/test-wpt.log/text
$ wget https://build.servo.org/builders/linux-rel-wpt/builds/11462/steps/test__1/logs/test-wpt.log/text
$ ./mach update-wpt text text.1
$ git add tests/wpt
$ git commit -a -m "Update test expectations"

Then:

  • add OffscreenCanvas to tests/wpt/mozilla/tests/mozilla/interfaces.js
  • run:
$ ./mach test-wpt --no-pause-after-test tests/wpt/mozilla/tests/mozilla/interfaces.html tests/wpt/mozilla/tests/mozilla/interfaces.worker.html --log-raw log
$ ./mach update-wpt log
$ git commit -a --amend --no-edit

And the resulting commit should contain all of the necessary test result changes.

@maharsh312

This comment has been minimized.

Copy link
Contributor Author

maharsh312 commented Jan 15, 2019

./mach update-wpt text text.1

I getting this erros while running "./mach update-qpt text text.1" : https://pastebin.com/vYHL6yah

@jdm Is this fine? Should I go ahead with other commands?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 15, 2019

@maharsh312 You may need to rebase your branch against master before you can run that command.

@maharsh312

This comment has been minimized.

Copy link
Contributor Author

maharsh312 commented Jan 15, 2019

@maharsh312 You may need to rebase your branch against master before you can run that command.

Did that. Now it is showing some test are not found

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 15, 2019

Let's ignore those warnings and see what happens ;)

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Jan 16, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1547597587939.

@maharsh312

This comment has been minimized.

Copy link
Contributor Author

maharsh312 commented Jan 16, 2019

@jdm I messed up rebasing the repo right?

@jdm jdm force-pushed the maharsh312:master branch from 637add5 to 11587e8 Jan 16, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

Yes. Here is what I did to fix it up starting from a pristine master branch:

$ git checkout -b offscreencanvas
git cherry-pick b4a77a5fea7026f388a80d24864a4fa375052096
git cherry-pick df1e451
git cherry-pick 30da91f
git cherry-pick a489778
git cherry-pick 042fa3d
git cherry-pick e845644
git cherry-pick 668a19c
git cherry-pick 637add5

This attached each of your commits, and I fixed up a merge conflict with the last one. Then I rebased to squash all of the commits into one. Then I force pushed the resulting commit over your master branch so the pull request was updated. I'm going to go ahead and try to merge this, because I would rather us not get stuck on trying to keep fixing this up.

@jdm jdm force-pushed the maharsh312:master branch from 11587e8 to 4ae3350 Jan 16, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

📌 Commit 4ae3350 has been approved by jdm

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

📌 Commit f564b24 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

⌛️ Testing commit f564b24 with merge 41181e3...

bors-servo added a commit that referenced this pull request Jan 16, 2019

Auto merge of #22495 - maharsh312:master, r=jdm
OffscreenCanvas API

<!-- Please describe your changes on the following line: -->
Initial Steps for OffscreenCanvas API are done in this commit.

r? @jdm
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14627 (GitHub issue number if applicable)

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/22495)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

💔 Test failed - linux-rel-wpt

@jdm jdm force-pushed the maharsh312:master branch from f564b24 to 0f17273 Jan 16, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

📌 Commit 0f17273 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

⌛️ Testing commit 0f17273 with merge f5cecfc...

bors-servo added a commit that referenced this pull request Jan 16, 2019

Auto merge of #22495 - maharsh312:master, r=jdm
OffscreenCanvas API

<!-- Please describe your changes on the following line: -->
Initial Steps for OffscreenCanvas API are done in this commit.

r? @jdm
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14627 (GitHub issue number if applicable)

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/22495)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

bors-servo added a commit that referenced this pull request Jan 16, 2019

Auto merge of #22495 - maharsh312:master, r=jdm
OffscreenCanvas API

<!-- Please describe your changes on the following line: -->
Initial Steps for OffscreenCanvas API are done in this commit.

r? @jdm
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14627 (GitHub issue number if applicable)

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/22495)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

⌛️ Testing commit 0f17273 with merge 09cdcab...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 16, 2019

@bors-servo bors-servo merged commit 0f17273 into servo:master Jan 16, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment