1189155 - Convert private tabbrowser properties to use the underscore prefix instead of "m"

Status: ASSIGNED

(Firefox :: Tabbed Browser, defect, P5)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This will be bitrotted once bug 486262 gets merged around, but it should be trivial to update.

Try push,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b4a80bb2fa

Comment 2

[Dão Gottwald [:dao]]

I've been going back and forth on this. I'm not really sure it's worthwhile. How long do you think we'd have to keep the deprecated aliases?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

We can use the default JetPack deprecation schedule[1], which is 18 weeks. If you want I can update the deprecation message to specify which Firefox version they will no longer be supported.

I feel pretty strongly that we should make this change. We can't keep letting add-ons hold back us trying to move forward.

[1] https://wiki.mozilla.org/Jetpack/Module_Deprecation_Process

Comment 4

[Dão Gottwald [:dao]]

Well, yes... but when I said I'm not sure it's worthwhile, I also implicitly meant that the obsolete "member" prefix probably doesn't hold us back significantly.

Did you do some research about how many add-ons would be affected here?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

A quick glance through add-ons MXR (didn't look like more was needed) shows that *a lot* of add-ons will be affected. I imagine a super majority of these add-ons will not be updated, and would break after the aliases are removed.

We could extend that deprecation period to 36 weeks or longer, but the risk of pushing out the deadline just means that it has no teeth and developers will just never update their add-on anyways.

The alternative would be to remove the deprecation warning and keep the aliases indefinitely. This would allow Firefox code to have a consistent and "future" coding style, while also maintaining full backwards compatibility at very low cost. What do you think about this alternative?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> The alternative would be to remove the deprecation warning and keep the
> aliases indefinitely. This would allow Firefox code to have a consistent and
> "future" coding style, while also maintaining full backwards compatibility
> at very low cost. What do you think about this alternative?

I think that's probably not worth the runtime overhead from the aliasing :/

It's definitely a small overhead, but the inconvenience from the m prefix isn't that big either.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8640903 [details] [diff] [review]
Patch

Worrying about the runtime overhead of an extra property call seems like premature optimization at its finest.

We do have to take in to account the cost of developers time here, and supporting two different naming schemes for member variables consumes extra time and leads to unnecessary bugs.

I'm redirecting the review to Gijs to hear his opinion.

Comment 8

[:Gijs (he/him)]

Hm. I don't know how big the "keeping us back" factor is when it's "just" about variable naming.

OTOH I feel pretty strongly that we need to do work to make things easier for contributors, and consistent code style is part of that.

I would feel better about this if it didn't introduce quite as much (as I imagine - AFAICT it's not in the patch?) "make all the things continue to work" code. Maybe we could dynamically create the getters from _init with a list instead of adding actual _properties.

This has the downside of those being expandos and therefore remaining when the binding is destroyed, but if they're just forwarding getters I don't think we need to care about this, and if add-ons depend on the presence or otherwise of those properties then they get to update...


Also, this is going to break all the uplifts. We'd have to do it *sometime* or live with it, I guess. :-\

Comment 9

[:Gijs (he/him)]

Comment on attachment 8640903 [details] [diff] [review]
Patch

I think if we're in doubt (which we seem to be), maybe this is a good time to ask the TAG. Mossop would be an obvious choice. :-)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #8)
> I would feel better about this if it didn't introduce quite as much (as I
> imagine - AFAICT it's not in the patch?) "make all the things continue to
> work" code.

The code is in the patch, search for "defineProperty".

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Dave, referring to comment #6 through #9, can you provide some input here? We're currently supporting two naming conventions for member variables (mPrefix and _prefix).

The patch attached to this bug switches us to use _prefix everywhere, while adding aliases for the old mPrefix style so we don't have to worry about breaking add-ons (it also includes a deprecation warning that we could remove if we aren't concerned about carrying around the backwards-compatibility).

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8640903 [details] [diff] [review]
Patch

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

Consistent code-style is good. Breaking add-ons is bad. This approach seems to balance the two nicely. I don't think we need to worry about the forwarding definitions remaining after tabbrowser loses its binding, if something is removing tabbrowser from the document then they'll have bigger problems to deal with. I can't imagine the overhead will be detectable at all.

But I don't think we could ever justify breaking a bunch of add-ons to turn off the old properties so while we could put a deprecation warning on them we can't time-limit that. That said I think we should still warn, it will help contributors spot when they've made a mistake and it should cause updated add-ons to fail reviews at AMO. It's also cheap to do. Pretty soon e10s and other things are likely to kill a lot of the unmaintained add-ons so we can revisit removing the old properties then.

::: browser/base/content/utilityOverlay.js
@@ +787,5 @@
> +      let Deprecated = Components.utils.import("resource://gre/modules/Deprecated.jsm", {}).Deprecated;
> +      let text = "`" + deprecatedName + "` is now deprecated, please use `" + newName + "`";
> +      Deprecated.warning(text, url);
> +      return thisObj[newName];
> +    },

You should probably provide a setter too.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

We can probably do this change in 57 since XUL addons will be no more, but I don't have the time to revive this patch.