Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

BrunoBernardino · 2017-08-09T11:48:27Z

Basic skeleton for implementing CanvasRenderingContext2d.fillText, only adding methods and parameters in the right place, and a basic test, with some println!().

This is only the beginning. It were my first couple of hours looking at Rust and Servo.

However, I have no clue how to get the text to render now (basically go from the println!() to something else).

It's also possible I messed something up with the DOMString.parse() but not entirely sure.

I'm doing this PR as a starting point to get help and learn more about this, or at least maybe save someone some time while implementing this, if no one's able to take the time and show me where/how.

Because it's still a work-in-progress, I'm leaving the boxes below unchecked (even though there are no errors).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11681 and #17963

There are tests for these changes

This change is

highfive · 2017-08-09T11:48:33Z

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

highfive · 2017-08-09T11:48:34Z

Heads up! This PR modifies the following files:

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

highfive · 2017-08-09T11:48:35Z

Warning

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

emilio · 2017-08-09T12:08:42Z

So first of all, this is awesome, thank you! :)

It's a bit unclear to me how easy it is to fill the missing pieces here. For the current 2d canvas APIs for now we use bindings to azure.

So, in theory, the only thing that we have to do is to expose the text drawing APIs azure has, and use them from here, with the current parameters (that is, font-size, etc).

That being said, I suspect this is not an easy task though. As far as I understand, all the text caches in azure aren't thread-safe, so it'd need a fair amount of work.

It'd be nice to try to use another back-end, but that's also a huge amount of work. If we want to implement this properly, I'd rather discuss with people what's the best long-term fix for this. I know @asajeffrey has been fiddling with Canvas2D for a while now, so maybe he has ideas. @glennw may also have an opinion on what's the best way to support text stuff in Canvas2D.

asajeffrey · 2017-08-09T18:44:41Z

I've talked to @pcwalton about using a back-end based on webrender + pathfinder, so I'm not sure how much effort we should put in to updating the current azure back end.

bors-servo · 2017-08-15T22:08:23Z

☔ The latest upstream changes (presumably #17891) made this pull request unmergeable. Please resolve the merge conflicts.

MortimerGoro · 2017-08-15T22:25:49Z

It may be interesting to provide a mock implementation for fillText or other missing Canvas2D methods before the long term webrender/pathfinder solution lands (any ETA on this? I'm assuming that it can take some time). Some WebGL demos that I try crash due to the missing methods and usually is to render a sprite not related to the game itself (e.g. the fps stats).

I'd prefer some "unimplemented method" warnings in the console instead of a crash in those cases. Also a lot of WebGL tests are exiting early because of the missing fillText() function, it would be good to enable them.

What do you think?

jdm · 2017-08-15T22:48:59Z

@MortimerGoro I am confused how a missing JS method could cause a crash/panic in Servo. Do you mean that the tests report a JS exception?

MortimerGoro · 2017-08-15T23:03:25Z

@jdm Yes, I meant that JavaScript/game execution "crashes" with a uncaught JS exception, not the servo process.

jdm · 2017-08-15T23:11:20Z

I would be open to adding stubs for missing canvas APIs that are hidden behind a preference.

bors-servo · 2017-08-17T06:30:40Z

☔ The latest upstream changes (presumably #18118) made this pull request unmergeable. Please resolve the merge conflicts.

BrunoBernardino · 2017-08-18T08:32:58Z

Can you elaborate on that @jdm ? Basically change my console.logs above to better console.warn and where would the preference be? (and how could I add it)?

jdm · 2017-08-18T15:41:08Z

Replace them with error!(...) instead; add a [Pref="dom.canvas-text.enabled"] to the CanvasRenderingContext2D.webidl file (example); add the new preference to resources/prefs.json.

BrunoBernardino · 2017-08-19T17:30:48Z

Not sure if this is what you were thinking about, and it doesn't really fix any of the mentioned issues, though, because they would require an actual implementation.

I do understand, as per @MortimerGoro's comment that it's fine and this will somewhat improve things.

Please let me know if I'm missing any other sort of unit tests or something.

emilio · 2017-08-19T17:34:57Z

components/script/dom/canvasrenderingcontext2d.rs

@@ -800,6 +800,13 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D {
        receiver.recv().unwrap()
    }

+    // https://html.spec.whatwg.org/multipage/#dom-context-2d-filltext
+    fn FillText(&self, text: DOMString, x: f64, y: f64, max_width: Option<f64>) {
+        let parsed_text: String = text.parse().unwrap();


This doesn't need parse(), you should be able to do into(), or if it doesn't exist, you may as well add it.

Thank you @emilio! I'll do that!

Oh. I'm afraid DOMString doesn't have into() and I'm not sure how to add it to components/script/dom/bindings/str.rs, can you help?

Apparently just .into() works. I was trying .into().unwrap(). Thanks!

bors-servo · 2017-08-22T22:13:44Z

☔ The latest upstream changes (presumably #18093) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-08-22T22:45:27Z

resources/prefs.json

@@ -1,7 +1,7 @@
 {
  "dom.bluetooth.enabled": false,
  "dom.bluetooth.testing.enabled": false,
-  "dom.canvas-text.enabled": false,
+  "dom.canvas-text.enabled": true,


Please revert this change. We do not want this to be exposed by default, since it is incomplete. Anybody that needs to run tests with this API present can enable it explicitly.

Sounds good! I thought the goal here was to prevent the JS Exception by default.

BrunoBernardino · 2017-08-23T09:43:34Z

Alright, I think that's it!

jdm · 2017-08-23T15:28:39Z

./resources/prefs.json:None: Unordered key (found dom.compositionevent.enabled before dom.canvas-text.enabled)

Otherwise, these changes look good. Could you squash all of the commits into one?

BrunoBernardino · 2017-08-23T15:48:21Z

Will do.


        Implement CanvasRenderingContext2d.fillText's "unimplemented" message

jdm · 2017-08-23T17:15:33Z

@bors-servo: r+
Thanks!

bors-servo · 2017-08-23T17:15:34Z

📌 Commit 2af8284 has been approved by jdm

bors-servo · 2017-08-23T17:48:11Z

⌛ Testing commit 2af8284 with merge a9f9207e4049979351313e6939f38e1eb7d07e8a...

bors-servo · 2017-08-23T18:23:39Z

💔 Test failed - linux-rel-wpt

jdm · 2017-08-23T18:33:13Z

@bors-servo: retry

#13342

bors-servo · 2017-08-23T19:00:49Z

⌛ Testing commit 2af8284 with merge acd08c7...


        Auto merge of #18020 - BrunoBernardino:feature-canvas-filltext, r=jdm

Implement CanvasRenderingContext2d.fillText's "unimplemented" message Basic skeleton for implementing CanvasRenderingContext2d.fillText, only adding methods and parameters in the right place, and a basic test, with some `println!()`.  This is only the beginning. It were my first couple of hours looking at Rust and Servo. However, I have _no clue_ how to get the text to render now (basically go from the `println!()` to something else). It's also possible I messed something up with the `DOMString.parse()` but not entirely sure. I'm doing this PR as a starting point to get help and learn more about this, _or_ at least maybe save someone some time while implementing this, if no one's able to take the time and show me where/how. Because it's still a work-in-progress, I'm leaving the boxes below unchecked (even though there are no errors). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #11681 and #17963  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18020)

bors-servo · 2017-08-23T20:06:08Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing acd08c7 to master...

highfive assigned metajack Aug 9, 2017

highfive added the S-awaiting-review label Aug 9, 2017

highfive added the S-needs-rebase label Aug 15, 2017

highfive added the S-needs-rebase label Aug 17, 2017

emilio reviewed Aug 19, 2017

View changes

BrunoBernardino changed the title ~~Implement CanvasRenderingContext2d.fillText - WIP~~ Implement CanvasRenderingContext2d.fillText's "unimplemented" message Aug 21, 2017

highfive added the S-needs-rebase label Aug 22, 2017

jdm reviewed Aug 22, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 22, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 23, 2017

jdm added the S-fails-tidy label Aug 23, 2017

jdm added S-needs-squash and removed S-awaiting-review S-needs-rebase labels Aug 23, 2017

Implement CanvasRenderingContext2d.fillText's "unimplemented" message

Unverified

No user is associated with the committer email.

Learn about signing commits

Loading status checks…

2af8284

highfive added the S-awaiting-review label Aug 23, 2017

highfive assigned jdm and unassigned metajack Aug 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Aug 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 23, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 23, 2017

bors-servo merged commit 2af8284 into servo:master Aug 23, 2017
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

highfive removed the S-awaiting-merge label Aug 23, 2017

BrunoBernardino deleted the BrunoBernardino:feature-canvas-filltext branch Aug 24, 2017

atouchet mentioned this pull request Aug 29, 2017

Happy face is missing #17963

Closed

servo / servo

Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

BrunoBernardino commented Aug 9, 2017 •

edited

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

emilio commented Aug 9, 2017

asajeffrey commented Aug 9, 2017

bors-servo commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017 •

edited

jdm commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017

jdm commented Aug 15, 2017

bors-servo commented Aug 17, 2017

BrunoBernardino commented Aug 18, 2017

jdm commented Aug 18, 2017 •

edited

BrunoBernardino commented Aug 19, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 22, 2017

This comment has been minimized.

This comment has been minimized.

BrunoBernardino commented Aug 23, 2017

jdm commented Aug 23, 2017

BrunoBernardino commented Aug 23, 2017

jdm commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

jdm commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

servo / servo

Join GitHub today

Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

Conversation

BrunoBernardino commented Aug 9, 2017 • edited

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

emilio commented Aug 9, 2017

asajeffrey commented Aug 9, 2017

bors-servo commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017 • edited

jdm commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017

jdm commented Aug 15, 2017

bors-servo commented Aug 17, 2017

BrunoBernardino commented Aug 18, 2017

jdm commented Aug 18, 2017 • edited

BrunoBernardino commented Aug 19, 2017

This comment has been minimized.

emilio Aug 19, 2017

This comment has been minimized.

BrunoBernardino Aug 19, 2017

This comment has been minimized.

BrunoBernardino Aug 19, 2017

This comment has been minimized.

BrunoBernardino Aug 19, 2017

bors-servo commented Aug 22, 2017

This comment has been minimized.

jdm Aug 22, 2017

This comment has been minimized.

BrunoBernardino Aug 23, 2017

BrunoBernardino commented Aug 23, 2017

jdm commented Aug 23, 2017

BrunoBernardino commented Aug 23, 2017

jdm commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

jdm commented Aug 23, 2017

bors-servo commented Aug 23, 2017

bors-servo commented Aug 23, 2017

BrunoBernardino commented Aug 9, 2017 •

edited

MortimerGoro commented Aug 15, 2017 •

edited

jdm commented Aug 18, 2017 •

edited