Many changes to DictionaryValues:...

Many changes to DictionaryValues:
* Add support for keys with "." in them via new XXXWithoutPathExpansion() APIs.
* Use these APIs with all key iterator usage.
* SetXXX() calls cannot fail, so change them from bool to void.
* Change GetSize() to size() since it's cheap, and add empty().

Other:
* Use standard for loop format in more places (e.g. instead of while loops when they're really doing a for loop).
* Shorten a few bits of code.

BUG=567
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33109

[tony, 2009-11-25 04:53:56]

http://codereview.chromium.org/441008/diff/2001/3006
File base/json/json_reader_unittest.cc (right):

http://codereview.chromium.org/441008/diff/2001/3006#newcode377
base/json/json_reader_unittest.cc:377: 
Can you add a test for something like { "a.b" : 1, "a" : { "b": 2 } }?

http://codereview.chromium.org/441008/diff/2001/3002
File base/values.cc (right):

http://codereview.chromium.org/441008/diff/2001/3002#newcode279
base/values.cc:279: entry->Set(remaining_path, in_value);
I'm a bit worried about someone crafting a URL or some other key value with lots
of dots (say, hundreds).  I think in that case you can overflow the stack.  Can
we do this in a loop instead of using recursion?

http://codereview.chromium.org/441008/diff/2001/3017
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/441008/diff/2001/3017#newcode223
chrome/browser/extensions/extension_prefs.cc:223: std::string id =
WideToASCII(*extension_id);
What is this for?

http://codereview.chromium.org/441008/diff/2001/3031
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/441008/diff/2001/3031#newcode779
chrome/common/extensions/extension.cc:779: if
(!colors_value->GetListWithoutPathExpansion(*iter, &color_list) ||
Nit: The !... || is a bit confusing.  Why not
(colors_value->(...) &&

Actually, this whole thing is confusing.  I think a comment would help me out
more than anything else.

[Peter Kasting, 2009-11-25 05:10:00]

Will address your comments tomorrow.

http://codereview.chromium.org/441008/diff/2001/3002
File base/values.cc (right):

http://codereview.chromium.org/441008/diff/2001/3002#newcode279
base/values.cc:279: entry->Set(remaining_path, in_value);
On 2009/11/25 04:53:56, tony wrote:
> I'm a bit worried about someone crafting a URL or some other key value with
lots
> of dots (say, hundreds).  I think in that case you can overflow the stack. 
Can
> we do this in a loop instead of using recursion?

Sure.  Note though that this is nearly identical to the current code, which
suffers just as much from this problem already.

[Peter Kasting, 2009-11-25 19:16:26]

http://codereview.chromium.org/441008/diff/2001/3006
File base/json/json_reader_unittest.cc (right):

http://codereview.chromium.org/441008/diff/2001/3006#newcode377
base/json/json_reader_unittest.cc:377: 
On 2009/11/25 04:53:56, tony wrote:
> Can you add a test for something like { "a.b" : 1, "a" : { "b": 2 } }?

Done.  Also added a parallel writer test.

http://codereview.chromium.org/441008/diff/2001/3002
File base/values.cc (right):

http://codereview.chromium.org/441008/diff/2001/3002#newcode279
base/values.cc:279: entry->Set(remaining_path, in_value);
On 2009/11/25 04:53:56, tony wrote:
> I'm a bit worried about someone crafting a URL or some other key value with
lots
> of dots (say, hundreds).  I think in that case you can overflow the stack. 
Can
> we do this in a loop instead of using recursion?

Done.  Also fixed Get() and Remove().

http://codereview.chromium.org/441008/diff/2001/3017
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/441008/diff/2001/3017#newcode223
chrome/browser/extensions/extension_prefs.cc:223: std::string id =
WideToASCII(*extension_id);
On 2009/11/25 04:53:56, tony wrote:
> What is this for?

It's just a statement from the old code, moved below the above block since it's
not needed during that block.

http://codereview.chromium.org/441008/diff/2001/3031
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/441008/diff/2001/3031#newcode779
chrome/common/extensions/extension.cc:779: if
(!colors_value->GetListWithoutPathExpansion(*iter, &color_list) ||
On 2009/11/25 04:53:56, tony wrote:
> Nit: The !... || is a bit confusing.  Why not
> (colors_value->(...) &&
> 
> Actually, this whole thing is confusing.  I think a comment would help me out
> more than anything else.

Added some comments.

[tony, 2009-11-25 19:30:13]

LGTM!

http://codereview.chromium.org/441008/diff/5036/4003
File base/values.cc (right):

http://codereview.chromium.org/441008/diff/5036/4003#newcode494
base/values.cc:494: return
current_dictionary->RemoveWithoutPathExpansion(current_path, out_value);
Nit: 80 cols