1296173 - stylo: Propose changing naming convention of servo binding functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 1

[Cameron McCormack (:heycam)]

I'm in favour of a consistent function naming scheme, and this one sounds good to me.

Comment 2

[Bobby Holley (:bholley)]

SGTM. Likely to be a disruptive change, so please coordinate the landing carefully.

Comment 3

[Manish Goregaokar [:manishearth]]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(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".

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(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 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1296173 part 1 - Change StyleSet manipulation functions to be prefixed by Servo_StyleSet_.

Review commit: https://reviewboard.mozilla.org/r/73474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73474/

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1296173 part 2 - Rename the servo binding functions.

Review commit: https://reviewboard.mozilla.org/r/73476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73476/

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

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

https://reviewboard.mozilla.org/r/73476/#review71512

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/550c3f778e15
https://hg.mozilla.org/mozilla-central/rev/1490d5840149