Skip to content

Conversation

garretrieger
Copy link
Collaborator

Makes some changes to the hb-subset.h API before we stabilize it:

  • Remove individual get/set methods for bool properties and instead use an enum of property names.
  • hb_subset_input is now const on the hb_subset() call.
  • Add hb_subset_or_fail ()
  • Adds set/get user data methods that are standard on hb_objects.
  • Adds passthrough tables and no subset tables to match fontTools table dropping and retention logic.

I've still got a couple things left to do before this is ready:

  • Documentation on the properties enum.
  • Documentation for all methods.
  • Remove hb_subset() in favor of hb_subset_or_fail()
  • Re-order get/set user data methods.

@behdad
Copy link
Member

behdad commented Jun 11, 2021

Thanks Garret!

CC @jfkthame @drott @bungeman @mapreri #3012

@khaledhosny khaledhosny added the subset hb-subset related bugs label Jun 11, 2021
@drott
Copy link
Collaborator

drott commented Jun 23, 2021

Thanks, @garretrieger! API change generally LGTM but I am not too familiar with the API and its usage. third_party/skia/src/pdf/SkPDFSubsetFont.cpp owners are better suited to answer.

What's hb_subset_input_set_user_data used for?

IIUC this will need a migration off of hb_subset_input_set_retain_gids to hb_subset_set_flags in Skia & Chromium. Usually the way that's done is to define a compile flag in Skia, set this compile flag in Chromium's top level skia/BUILD.gn defines list when rolling HarfBuzz in Chromium. Are you up for handling that migration?

@behdad
Copy link
Member

behdad commented Jun 23, 2021

What's hb_subset_input_set_user_data used for?

That's generic part of HB object model; so you can attach arbitrary (say, SkFace) data to a HB object.

@behdad
Copy link
Member

behdad commented Jun 23, 2021

That's generic part of HB object model; so you can attach arbitrary (say, SkFace) data to a HB object.

Sometimes used by language bindings.

@garretrieger
Copy link
Collaborator Author

IIUC this will need a migration off of hb_subset_input_set_retain_gids to hb_subset_set_flags in Skia & Chromium. Usually the way that's done is to define a compile flag in Skia, set this compile flag in Chromium's top level skia/BUILD.gn defines list when rolling HarfBuzz in Chromium. Are you up for handling that migration?

Yes I can try and do that migration.

Copy link
Collaborator

@khaledhosny khaledhosny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think subset documentation is currently generated, appropriate section need to be added to the files unders /docs.

@behdad
Copy link
Member

behdad commented Jun 30, 2021

Thanks. Garret, can you also add a SECTION:hb-subset comment to hb-subset.cc to provide some description for this section of the docs? See top of hb-shape.cc for example.

@garretrieger
Copy link
Collaborator Author

Thanks. Garret, can you also add a SECTION:hb-subset comment to hb-subset.cc to provide some description for this section of the docs? See top of hb-shape.cc for example.

Done.

@garretrieger garretrieger changed the title [subset] WIP proposed stable API for hb-subset.h [subset] update hb_subset api with final changes before going stable. Jul 22, 2021
@garretrieger garretrieger marked this pull request as ready for review July 22, 2021 23:36
@garretrieger
Copy link
Collaborator Author

I don't think subset documentation is currently generated, appropriate section need to be added to the files unders /docs.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subset hb-subset related bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants