stylo: Propose changing naming convention of servo binding functions

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
It seems the only current naming convention is that we use "Servo_" and "Gecko_" prefix to distinguish the side a function is implemented, and the rest of a function name is just a random verb phase. It doesn't seem to be very scalable, especially when we need more functions on some objects.

I suggest a naming convention that put the target type as a prefix as well, for example, currently we have:
* Servo_DropDeclarationBlock
* Servo_GetDeclarationBlockCache
* Servo_SetDeclarationBlockImmutable
* Servo_ClearDeclarationBlockCachePointer
We should change them to:
* Servo_DeclarationBlock_Drop
* Servo_DeclarationBlock_GetCache
* Servo_DeclarationBlock_SetImmutable
* Servo_DeclarationBlock_ClearCachePointer
which should be clearer for people to understand what type of object the function is operating on.

I'm proposing this because for CSSOM, we would need much more functions on some types. In my WIP patch, I'm adding the following functions:
* Servo_CreateEmptyDeclarationBlock
* Servo_GetCssTextOfDeclarationBlock
* Servo_GetLengthOfDeclarationBlock
* Servo_GetPropertyFromDeclarationBlock
* Servo_SetPropertyOfDeclarationBlock
* Servo_RemovePropertyFromDeclarationBlock
which do not look good to me, but if we change the naming convention, we would have:
* Servo_DeclarationBlock_Create
* Servo_DeclarationBlock_GetCssText
* Servo_DeclarationBlock_GetLength
* Servo_DeclarationBlock_GetProperty
* Servo_DeclarationBlock_SetProperty
* Servo_DeclarationBlock_RemoveProperty
I'm in favour of a consistent function naming scheme, and this one sounds good to me.
SGTM. Likely to be a disruptive change, so please coordinate the landing carefully.
SGTM.

I feel like a lot of our manual bindings could go away if we turned on bindgen's method generation for a few types (not sure if it can be configured that way yet), though msvc may not like those methods.
(In reply to Manish Goregaokar [:manishearth] from comment #3)
> I feel like a lot of our manual bindings could go away if we turned on
> bindgen's method generation for a few types (not sure if it can be
> configured that way yet), though msvc may not like those methods.

That should just work, as long as they're not extern "C".
(Assignee)

Comment 5

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> (In reply to Manish Goregaokar [:manishearth] from comment #3)
> > I feel like a lot of our manual bindings could go away if we turned on
> > bindgen's method generation for a few types (not sure if it can be
> > configured that way yet), though msvc may not like those methods.
> 
> That should just work, as long as they're not extern "C".

That would probably need build-time bindgen, otherwise we may not get the proper name mangling.

Also we would need Clang 3.9 for that since 3.8 doesn't fully support MSVC 2015.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> That would probably need build-time bindgen, otherwise we may not get the
> proper name mangling.
> 
> Also we would need Clang 3.9 for that since 3.8 doesn't fully support MSVC
> 2015.

Yeah, that's going to be a major annoyance until we're able to automatize the binding generation cross-platform, I agree. In spidermonkey we generate a set of bindings per platform instead of bindgen-at-buildtime, but that doesn't scale well with fast modifications, unfortunately.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
The binding functions implemented in the Gecko side are not renamed because as mentioned in comment 3, many of them can be replaced by an improved bindgen in the future. Also, most of the types only have one or two functions operating them, and it seems to me the number of those functions is unlikely to grow quickly, so applying the new convention wouldn't gain much.
(Assignee)

Updated

2 years ago
Assignee: nobody → xidorn+moz

Comment 10

2 years ago
mozreview-review
Comment on attachment 8783796 [details]
Bug 1296173 part 1 - Change StyleSet manipulation functions to be prefixed by Servo_StyleSet_.

https://reviewboard.mozilla.org/r/73474/#review71510
Attachment #8783796 - Flags: review?(bobbyholley) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8783797 [details]
Bug 1296173 part 2 - Rename the servo binding functions.

https://reviewboard.mozilla.org/r/73476/#review71512
Attachment #8783797 - Flags: review?(bobbyholley) → review+

Comment 12

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/550c3f778e15
part 1 - Change StyleSet manipulation functions to be prefixed by Servo_StyleSet_. r=bholley
https://hg.mozilla.org/integration/autoland/rev/1490d5840149
part 2 - Rename the servo binding functions. r=bholley

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/550c3f778e15
https://hg.mozilla.org/mozilla-central/rev/1490d5840149
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.