Open Bug 1189155 Opened 7 years ago Updated 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P5)

defect
Points:
1

Tracking

()

ASSIGNED

People

(Reporter: jaws, Assigned: dao)

References

Details

Attachments

(1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8640903 - Flags: review?(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?
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
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?
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?
Flags: needinfo?(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.
Flags: needinfo?(dao)
Attachment #8640903 - Flags: review?(dao)
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.
Attachment #8640903 - Flags: review?(gijskruitbosch+bugs)
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 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. :-)
Attachment #8640903 - Flags: review?(gijskruitbosch+bugs)
(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".
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).
Flags: needinfo?(dtownsend)
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.
Flags: needinfo?(dtownsend)
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.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Depends on: 1445572
Depends on: 1415537
Keywords: meta
Depends on: 1415863
Attachment #8640903 - Attachment is obsolete: true
Blocks: 1387013
Summary: Convert tabbrowser-tab fields to use underscore prefix (deprecate "m" prefix) → Convert private tabbrowser properties to use the underscore prefix instead of "m"
Depends on: 1446060
Depends on: 1446314
Priority: -- → P5
Summary: Convert private tabbrowser properties to use the underscore prefix instead of "m" → [meta] Convert private tabbrowser properties to use the underscore prefix instead of "m"
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Keywords: meta
Summary: [meta] Convert private tabbrowser properties to use the underscore prefix instead of "m" → Convert private tabbrowser properties to use the underscore prefix instead of "m"
Points: --- → 1
You need to log in before you can comment on or make changes to this bug.