1285103 - Refactor TableUpdate to support V2 and V4

Status: RESOLVED DUPLICATE of bug 1284204

(Toolkit :: Safe Browsing, defect)

Description

[Henry Chang [:hchang]]

TableUpdate is a structure that carries the info to update the table. Even though the update instruction is essentially different between v2 and v4, it's still convenient to make TableUpdateV2 and TableUpdateV4 extend from the same base class. Besides, since RTTI is not allowed, we use a tag to implement a lightweight downcasting.

Comment 1

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1285103-Refactor-TableUpdate-to-support-v2-and-v.patch

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8768649 [details] [diff] [review]
0001-Bug-1285103-Refactor-TableUpdate-to-support-v2-and-v.patch

Hi Francois,

The main purpose of this patch is to make TableUpdate as abstract as possible so that it can be passed around functions for updating. To be honest, it's a little over-designed since there's few common behavior for TableUpdate v2 and v4. However, this tiny change makes the least intermediary code change. (For example, TableUpdate will be stored in somewhere and be passed to many other objects. If we don't have this abstract TableUpdate class, we might have to duplicate every single function for propagating TableUpdate.)

The actual TableUpdateV4 specific interface is not fully designed yet. But with this patch, I can start working on "generating" TableUpdateV4 and Dimi can start working on "applying TableUpdateV4". Both of us can gradually check in code based on this patch with no dependencies.

I am very glad to discuss this bug with you on irc if I haven't made everything clear enough. 

Thanks!

Comment 3

[François Marier [:francois]]

Comment on attachment 8768649 [details] [diff] [review]
0001-Bug-1285103-Refactor-TableUpdate-to-support-v2-and-v.patch

Review of attachment 8768649 [details] [diff] [review]:
-----------------------------------------------------------------

Should this be moved into and reviewed alongside another bug?

Comment 4

[Ethan Tseng [:ethan]]

The component is self-explained. Not necessary to identify SafeBrowsing in the title.