1245875 - Remove Tilt from the tree

Status: RESOLVED FIXED

(Firefox Graveyard :: Developer Tools: 3D View, defect)

Description

[Victor Porof [:vporof][:vp]]

We have an add-on.

Apparently there's not enough users to justify any potential maintenance, and no immediate need for implementing some ideas that we've been having (such as like z-index-based depth ordering, visualizing 3D CSS transforms, using some backend for VR-based web browsing and other things).

Comment 1

[Joe Walker [:jwalker]]

Will the add-on work with e10s enabled?
I've been assuming that current ongoing maintenance cost was close to zero, is this not right?

Comment 2

[Victor Porof [:vporof][:vp]]

We have e10s bugs to fix (Tilt relies heavily on directly accessing the content document), tests to update (same thing), and it was never updated to use the RDP. Since e10s is now in beta, if we don't fix everything soon Tilt won't be able to be turned on.

The add-on works in similar ways as the built-in implementation.

Comment 3

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7e4e0ba9d7

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Remove Titl from tree.

In case we actually want to do it. Here is the patches!
First part, simple. Just remove everything related to tilt.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Remove uncessary dependency on tilt from canvas debugger

And some other patch to not break things.
canvas debugger tests seem to have an unecessary dep en tilt.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Inline tilt dependency in shared editor.

Shared editor actually use tilt-gl.
So just inline the used function in there.
We could also keep that in a module in shared somewhere?

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8723643 [details] [diff] [review]
Remove uncessary dependency on tilt from canvas debugger

Review of attachment 8723643 [details] [diff] [review]:
-----------------------------------------------------------------

canvas debugger also works and tests webgl content.

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8723641 [details] [diff] [review]
Remove Titl from tree.

Review of attachment 8723641 [details] [diff] [review]:
-----------------------------------------------------------------

s/Titl/Tilt/

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8723644 [details] [diff] [review]
Inline tilt dependency in shared editor.

Review of attachment 8723644 [details] [diff] [review]:
-----------------------------------------------------------------

Should put those in utils.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8723643 [details] [diff] [review]
Remove uncessary dependency on tilt from canvas debugger

Review of attachment 8723643 [details] [diff] [review]:
-----------------------------------------------------------------

Are you aware that gRequiresWebGL is always false and never switched to true?
That's the reason why I'm removing this code.
As far as I undertand this code, we never reach the code that access TiltGL.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> Comment on attachment 8723643 [details] [diff] [review]
> Remove uncessary dependency on tilt from canvas debugger
> 
> Review of attachment 8723643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you aware that gRequiresWebGL is always false and never switched to true?
> That's the reason why I'm removing this code.
> As far as I undertand this code, we never reach the code that access TiltGL.

Ugh, that seems to be accidental. There are tests that have touch WebGL content, we should be checking if it's available first.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Factor out webgl helpers out of tilt

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> Ugh, that seems to be accidental. There are tests that have touch WebGL
> content, we should be checking if it's available first.

Ah, ok that makes sense now.
Here is a new patch with a new module and canvas and shadereditor head.js use it.

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8723641 [details] [diff] [review]
Remove Titl from tree.

Joe, Can you officialy confirm this removal?

Do we need to communicate first? After?
Is there anything we should do before? After?

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8723641 [details] [diff] [review]

> Is there anything we should do before? After?

Apart from weeping?

Comment 15

[Victor Porof [:vporof][:vp]]

Comment on attachment 8724407 [details] [diff] [review]
Factor out webgl helpers out of tilt

Review of attachment 8724407 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/webgl-utils.js
@@ +48,5 @@
> +    !isWebGLForceEnabled() &&
> +     isWebGLSupportedByGFX() &&
> +     create3DContext(createCanvas());
> +
> +  info("Apparently, WebGL is" + (supported ? "" : " not") + " supported.");

Wouldn't `info` be undefined here?

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8723641 [details] [diff] [review]
Remove Titl from tree.

Review of attachment 8723641 [details] [diff] [review]:
-----------------------------------------------------------------

Less important, but there's still a reference you forgot to remove in .eslintignore.

https://dxr.mozilla.org/mozilla-central/source/.eslintignore#109

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> ::: devtools/client/shared/webgl-utils.js
> > +  info("Apparently, WebGL is" + (supported ? "" : " not") + " supported.");
> 
> Wouldn't `info` be undefined here?

Yes.. I removed this line.

(In reply to Tim Nguyen [:ntim] from comment #16)
> Comment on attachment 8723641 [details] [diff] [review]
> Less important, but there's still a reference you forgot to remove in
> .eslintignore.
> 
> https://dxr.mozilla.org/mozilla-central/source/.eslintignore#109

Thanks, I removed it.

Comment 18

[Alexandre Poirot [:ochameau]]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6e0db62837

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Remove Tilt from tree.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Factor out webgl helpers out of tilt

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8724684 [details] [diff] [review]
Remove Tilt from tree.

Review of attachment 8724684 [details] [diff] [review]:
-----------------------------------------------------------------

::: .eslintignore
@@ +103,5 @@
>  devtools/client/scratchpad/**
>  devtools/client/shadereditor/**
>  devtools/client/shared/**
>  devtools/client/sourceeditor/**
> +devtools/client/storage/**

You're reintroducing a line I've removed in bug 1251720.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Remove Tilt from tree.

I must have chosen the wrong git hash, I've seen this esignore mistake and removed it...
Here we go.

(thanks for watching my diff!)

Comment 23

[Alexandre Poirot [:ochameau]]

What about comment 13?

Comment 24

[Joe Walker [:jwalker]]

(In reply to Alexandre Poirot [:ochameau] from comment #23)
> What about comment 13?

I'm thinking that we should add it to the release notes and the hacks post.
Are there other things you think we should do?

Comment 25

[Alexandre Poirot [:ochameau]]

Release notes is a good idea.
I was wondering if we should have an explicit message on the mailing list with possible alternatives (the addon without e10s? Does it still work?) or some other medium?

Comment 26

[Joe Walker [:jwalker]]

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Release notes is a good idea.
> I was wondering if we should have an explicit message on the mailing list
> with possible alternatives (the addon without e10s? Does it still work?) or
> some other medium?

I agree, and I did that as part of a bigger G||D email.

Also good call :ntim on dev-doc-needed

Comment 27

[Alexandre Poirot [:ochameau]]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Mailing list post about that:
https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/sGoju9FQBlY

Comment 28

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/bf85f87b5d3d7d1653f4ca1e92c65af045ff3599
Bug 1245875 - Factor out webgl helpers out of tilt. r=vporof

https://hg.mozilla.org/integration/fx-team/rev/ca53292a4ca31712c9c826296d90d231f8a0bca2
Bug 1245875 - Remove Tilt from tree. r=vporof,jwalker

Comment 29

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/d3bd4762581e1bf0dae4c17acd1a7ff65dccc299
Bug 1245875 - Add missing createCanvas function in webgl-utils to fix mochitest-dt5 failures. r=me

Comment 30

[Alexandre Poirot [:ochameau]]

Attachment: Add missing createCanvas function in webgl-utils to fix mochitest-dt5 failures.

I was pretty sure I had some try build with the last patches, but looks like no.
Pushed an hotfix to keep fx-team green'n open.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf85f87b5d3d
https://hg.mozilla.org/mozilla-central/rev/ca53292a4ca3
https://hg.mozilla.org/mozilla-central/rev/d3bd4762581e

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (Aurora) release notes.

Comment 33

[Will Bamberg [:wbamberg]]

docs update: https://developer.mozilla.org/en-US/docs/Tools/3D_View

Comment 34

[Sebastian Zartner [:sebo]]

Ritu, I believe the release note should mention that Tilt is available as extension on AMO[1] instead.

Sebastian

[1] https://addons.mozilla.org/firefox/addon/tilt/

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Sebastian Zartner [:sebo] from comment #34)
> Ritu, I believe the release note should mention that Tilt is available as
> extension on AMO[1] instead.
> 
> Sebastian
> 
> [1] https://addons.mozilla.org/firefox/addon/tilt/

Done, relnote wording has been updated! You should see it reflected on Fx47 release notes within the hour.

Comment 36

[rugk]

However the add-on us really outdated, so it is really sad that this was removed from Firefox. Last update of the add-on: 2012.
So please also maintain the add-on if you remove it from Firefox.