I didn't realize we hadn't done this yet. I thought we were putting them in the entity metadata, but looking through the code, I don't see anywhere that does it.

This seems like a pretty important thing to get in. Without this, we can break forwards compatibility with future versions that introduce new fields (although granted it's rare to have a field that needs to be preserved in the face of a local update)

Bumping priority. 

Some more details or pointers about the requirements would be appreciated :-)

If they don't exist already, I would start with introducing browser tests that capture the desired external behavior.

Based on the unit tests that I can see, e.g.  BaseNodeSetSpecificsPreservesUnknownFields, introduced with  crbug.com/83982  long time ago, I see no specific scenario described.

A scenario I can guess is:
1. A bookmark exists sync-ed across two distinct versions of Chrome, one having support for new proto fields.
2. A local modification is done in the old client, say, change the name.
3. The new client would want the field unmodified.

It however seems a stretch that propagating an unknown field is a universal solution (e.g. if the new field is "last accessed time"), but I guess it's the best we can do, and consistent with pre-USS clients.

Yeah, that's exactly right. In theory any new field like that won't be critical (since new entities created by old clients will definitely not have it filled), but it would still be good to preserve if we can.

Readjusting priority after realizing this is not what I thought it was, and the example above might be totally off.

Unknown proto fields in the actual datatype specifics (e.g. PrinterSpecifics) are responsibility of the bridge implementation and the model itself, hence we'd need to file individual bugs if this regressed in the process of migrating them to USS, because the APIs (including ModelTypeStore) actually support this well. pavely@: please help asses the situation.

For example, taking a look at PrintersSyncBridge, it seems like  PrinterToSpecifics() needs to be fixed to begin with.

On the other hand, what this bug's description suggests, based on the reference to WriteNode::SetEntitySpecifics, is specifically the unknown proto fields for the message EntitySpecifics. This seems less important (and unrelated to the scenario described earlier), and probably involves extending syncer::EntityData to keep unknown field values around.

As per my comment above, and regarding the specific datatypes, it'd be nice that we would add test coverage for unknown proto field handling to all USS datatypes, via a common test suite: crbug.com/796534

Triage ping: Any updates on this? Seems like we'd really want to have this before launching any major data type (like BOOKMARKS) with USS.

Triage ping again: Is this still relevant?

I have mixed feelings about this bug, but marking it as candidate for our upcoming fixit.

I took another look at decided in doesn't make much sense. Rationale:
1. Unknown fields directly within EntitySpecifics (being a oneof, effectively) almost certainly means an unknown datatype, hence there's no point is bothering.

2. Unknown fields in nested messages (e.g. BookmarkSpecifics) are hard to implement. It is a much more likely scenario, but is anyway not supported by legacy clients either (directory), which I verified by locally modifying test BaseNodeSetSpecificsPreservesUnknownFields.