[webkit-reviews] review requested: [Bug 28964] [Chromium] ChromiumDataObject should have getter/setter interface : [Attachment 40061] patch - extend getter/setter interface

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 24 05:49:19 PDT 2009


Roland Steiner <rolandsteiner at google.com> has asked  for review:
Bug 28964: [Chromium] ChromiumDataObject should have getter/setter interface
https://bugs.webkit.org/show_bug.cgi?id=28964

Attachment 40061: patch - extend getter/setter interface
https://bugs.webkit.org/attachment.cgi?id=40061&action=review

------- Additional Comments from Roland Steiner <rolandsteiner at google.com>
(Sorry for adding to this after the patch has already landed.)

During further implementation with the interface in the original patch I found
that it is, while technically sufficient, perhaps a tad too simplistic, esp.
when considering that the interface should stay largely unchanged even if the
underlying data members change. In detail:

- The methods for file names are too reliant on the data member actually being
a Vector<String>.
  Manipulation of the list of filenames requires wholesale copying or using a
swapping trick, neither of which is very elegant.
- Several places of the code are only interested in whether a given member
exists, not what its actual value is.
  In these cases it is inefficient to have the code create a duplicate that is
soon discarded.

Therefore I added a new patch that extends the interface somewhat to try to
make it more useful and robust to further changes (*cough*). From the
ChangeLog:

- added contains...() methods to just query the state
- added containsValid...URL() methods for URL data members
- removed takeFileNames() as this was too type-dependent
- changed return type of fileNames() to Vector<String>
- added interface methods to allow appending to and iteration over file names


More information about the webkit-reviews mailing list