Correct const accessors in base/values.(h|cc)

Correct const accessors in base/values.(h|cc)

For problem description and other info please see the BUG page.
This is for DictionaryValue.

BUG=138946
TEST=N/A (no fix & no new feature)
TBR=jar scottbyer achuith agl mnissler davemoore garykac akalin hans bulach phajdan.jr jamesr

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

[vabr (Chromium), 2012-07-25 18:00:53]

Hi all,

Could you please review the parts for which you are OWNERS?

This is a first step from a clean-up described in the associated BUG, second
step will be similar, but for ListValues (in a different CL).

Mostly this is just adding "const", but there are two non-trivial places:
* Changes in base/values.(c|hh).
* In chrome/test/webdriver, webdriver::Command::parameters_ needed to lose
"const" from its declaration. It is because it was being used to modify the
pointed DictionaryValue, which was only possible due to the issue with
DictionaryValue accessors.

I added the majority of OWNERS as TBR, as the corresponding changes are just
adding const, which can be proved to work by compilability. I still need to hear
from somebody from base, chrome/browser/extensions, and chrome/test/webdriver
OWNERS, ideally brettw, yoz and kkania.

Looking forward to your comments,

Vaclav

[brettw, 2012-07-25 21:11:54]

base LGTM (didn't look at the rest).

[kkania, 2012-07-25 21:23:45]

Where/how is webdriver::Command::parameters_ being modified?

[Yoyo Zhou, 2012-07-25 22:20:39]

extensions LGTM

[vabr (Chromium), 2012-07-26 07:13:44]

On 2012/07/25 21:23:45, kkania wrote:
> Where/how is webdriver::Command::parameters_ being modified?

First, Command::GetDictionaryParameter exposes parts of Command::parameters_.
Then, in CookieCommand::ExecutePost, cookie_dict is taken from
Command::GetDictionaryParameter, and cookie_dict->SetString is called. This
triggers change back in the const Command::parameters_.

[kkania, 2012-07-26 15:28:11]

On 2012/07/26 07:13:44, vabr (Chromium) wrote:
> On 2012/07/25 21:23:45, kkania wrote:
> > Where/how is webdriver::Command::parameters_ being modified?
> 
> First, Command::GetDictionaryParameter exposes parts of Command::parameters_.
> Then, in CookieCommand::ExecutePost, cookie_dict is taken from
> Command::GetDictionaryParameter, and cookie_dict->SetString is called. This
> triggers change back in the const Command::parameters_.

I see. I think that code (Command::GetDictionaryParameter and
Cookie::ExecutePost) should be changed and the parameters_ left const. In the
cookie method, just duplicate the dict or merge it with a local dict.

[vabr (Chromium), 2012-07-26 20:43:10]

On 2012/07/26 15:28:11, kkania wrote:
> On 2012/07/26 07:13:44, vabr (Chromium) wrote:
> > On 2012/07/25 21:23:45, kkania wrote:
> > > Where/how is webdriver::Command::parameters_ being modified?
> > 
> > First, Command::GetDictionaryParameter exposes parts of
Command::parameters_.
> > Then, in CookieCommand::ExecutePost, cookie_dict is taken from
> > Command::GetDictionaryParameter, and cookie_dict->SetString is called. This
> > triggers change back in the const Command::parameters_.
> 
> I see. I think that code (Command::GetDictionaryParameter and
> Cookie::ExecutePost) should be changed and the parameters_ left const. In the
> cookie method, just duplicate the dict or merge it with a local dict.

Thanks for that insight.
Unfortunately it is a bit tricky to make local copies, because
CookieCommand::ExecutePost gives cookie_dict to Session:SetCookie, which posts
an asynchronous task. Does the cookie_dict get deep-copied during creating this?
If you are 100% sure that it does and it is OK to make a local copy, I'll do
that.

But otherwise I would suggest a conservative solution:
1. Submitting this CL with non-const parameters_, because this is not changing
how the code worked until now (only we are clear about not keeping it const).
2. Then I will file a bug about making parameters_ const again, and describe all
the details I know about it in there.
3. Somebody will tackle the problem from that bug. I can volunteer for that
because I would enjoy to learn another part of Chrome, but I cannot guarantee I
will do it quickly, other issues piled up on me.

I prefer the second option (with filing the bug) both because it separates two
different issues, and this CL is already full of changes from the first issue.
It will also help me to finish the other task which is waiting for fixing the
const handling.

What do you think about this?

Thanks,
Vaclav

[vabr (Chromium), 2012-07-26 20:48:02]

To kkania@:
I added some comments at the places I talked about, mainly for my own reference.
Vaclav

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:52: if
(!GetDictionaryParameter("cookie", &cookie_dict)) {
Here we get a sub-dictionary cookie_dict from parameters_.

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:68:
cookie_dict->SetString("domain", split_domain[0]);
Here we modify the sub-dictionary cookie_dict of the supposedly const dictionary
parameters_.

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:75: error =
session_->SetCookie(url, cookie_dict);
Here we pass cookie_dict to SetCookie, which will post the asynchronous task.

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_session.cc (right):

http://codereview.chromium.org/10834004/diff/7068/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_session.cc:586: cookie_dict,
Does this deep-copy cookie_dict, or does it rather only copy the direct data
members (pointers to allocated data) of the object?

[kkania, 2012-07-26 20:51:05]

On 2012/07/26 20:43:10, vabr (Chromium) wrote:
> On 2012/07/26 15:28:11, kkania wrote:
> > On 2012/07/26 07:13:44, vabr (Chromium) wrote:
> > > On 2012/07/25 21:23:45, kkania wrote:
> > > > Where/how is webdriver::Command::parameters_ being modified?
> > > 
> > > First, Command::GetDictionaryParameter exposes parts of
> Command::parameters_.
> > > Then, in CookieCommand::ExecutePost, cookie_dict is taken from
> > > Command::GetDictionaryParameter, and cookie_dict->SetString is called.
This
> > > triggers change back in the const Command::parameters_.
> > 
> > I see. I think that code (Command::GetDictionaryParameter and
> > Cookie::ExecutePost) should be changed and the parameters_ left const. In
the
> > cookie method, just duplicate the dict or merge it with a local dict.
> 
> Thanks for that insight.
> Unfortunately it is a bit tricky to make local copies, because
> CookieCommand::ExecutePost gives cookie_dict to Session:SetCookie, which posts
> an asynchronous task. Does the cookie_dict get deep-copied during creating
this?
> If you are 100% sure that it does and it is OK to make a local copy, I'll do
> that.
> 
> But otherwise I would suggest a conservative solution:
> 1. Submitting this CL with non-const parameters_, because this is not changing
> how the code worked until now (only we are clear about not keeping it const).
> 2. Then I will file a bug about making parameters_ const again, and describe
all
> the details I know about it in there.
> 3. Somebody will tackle the problem from that bug. I can volunteer for that
> because I would enjoy to learn another part of Chrome, but I cannot guarantee
I
> will do it quickly, other issues piled up on me.
> 
> I prefer the second option (with filing the bug) both because it separates two
> different issues, and this CL is already full of changes from the first issue.
> It will also help me to finish the other task which is waiting for fixing the
> const handling.
> 
> What do you think about this?
> 
> Thanks,
> Vaclav

Although it uses an asynchronous task underneath, Session::SetCookie is
guaranteed to be synchronous, so having the local copy is ok.

[vabr (Chromium), 2012-07-26 20:56:07]

On 2012/07/26 20:51:05, kkania wrote:
> On 2012/07/26 20:43:10, vabr (Chromium) wrote:
> > On 2012/07/26 15:28:11, kkania wrote:
> > > On 2012/07/26 07:13:44, vabr (Chromium) wrote:
> > > > On 2012/07/25 21:23:45, kkania wrote:
> > > > > Where/how is webdriver::Command::parameters_ being modified?
> > > > 
> > > > First, Command::GetDictionaryParameter exposes parts of
> > Command::parameters_.
> > > > Then, in CookieCommand::ExecutePost, cookie_dict is taken from
> > > > Command::GetDictionaryParameter, and cookie_dict->SetString is called.
> This
> > > > triggers change back in the const Command::parameters_.
> > > 
> > > I see. I think that code (Command::GetDictionaryParameter and
> > > Cookie::ExecutePost) should be changed and the parameters_ left const. In
> the
> > > cookie method, just duplicate the dict or merge it with a local dict.
> > 
> > Thanks for that insight.
> > Unfortunately it is a bit tricky to make local copies, because
> > CookieCommand::ExecutePost gives cookie_dict to Session:SetCookie, which
posts
> > an asynchronous task. Does the cookie_dict get deep-copied during creating
> this?
> > If you are 100% sure that it does and it is OK to make a local copy, I'll do
> > that.
> > 
> > But otherwise I would suggest a conservative solution:
> > 1. Submitting this CL with non-const parameters_, because this is not
changing
> > how the code worked until now (only we are clear about not keeping it
const).
> > 2. Then I will file a bug about making parameters_ const again, and describe
> all
> > the details I know about it in there.
> > 3. Somebody will tackle the problem from that bug. I can volunteer for that
> > because I would enjoy to learn another part of Chrome, but I cannot
guarantee
> I
> > will do it quickly, other issues piled up on me.
> > 
> > I prefer the second option (with filing the bug) both because it separates
two
> > different issues, and this CL is already full of changes from the first
issue.
> > It will also help me to finish the other task which is waiting for fixing
the
> > const handling.
> > 
> > What do you think about this?
> > 
> > Thanks,
> > Vaclav
> 
> Although it uses an asynchronous task underneath, Session::SetCookie is
> guaranteed to be synchronous, so having the local copy is ok.

OK, I'll do that in the morning (my timezone is GMT+1).
As it is quite clear to me what to do, are you fine if I do it, submit to CL and
TBR you, or do you want to see it before it goes to CL?
Cheers,
Vaclav

[kkania, 2012-07-26 21:06:24]

On 2012/07/26 20:56:07, vabr (Chromium) wrote:
> On 2012/07/26 20:51:05, kkania wrote:
> > On 2012/07/26 20:43:10, vabr (Chromium) wrote:
> > > On 2012/07/26 15:28:11, kkania wrote:
> > > > On 2012/07/26 07:13:44, vabr (Chromium) wrote:
> > > > > On 2012/07/25 21:23:45, kkania wrote:
> > > > > > Where/how is webdriver::Command::parameters_ being modified?
> > > > > 
> > > > > First, Command::GetDictionaryParameter exposes parts of
> > > Command::parameters_.
> > > > > Then, in CookieCommand::ExecutePost, cookie_dict is taken from
> > > > > Command::GetDictionaryParameter, and cookie_dict->SetString is called.
> > This
> > > > > triggers change back in the const Command::parameters_.
> > > > 
> > > > I see. I think that code (Command::GetDictionaryParameter and
> > > > Cookie::ExecutePost) should be changed and the parameters_ left const.
In
> > the
> > > > cookie method, just duplicate the dict or merge it with a local dict.
> > > 
> > > Thanks for that insight.
> > > Unfortunately it is a bit tricky to make local copies, because
> > > CookieCommand::ExecutePost gives cookie_dict to Session:SetCookie, which
> posts
> > > an asynchronous task. Does the cookie_dict get deep-copied during creating
> > this?
> > > If you are 100% sure that it does and it is OK to make a local copy, I'll
do
> > > that.
> > > 
> > > But otherwise I would suggest a conservative solution:
> > > 1. Submitting this CL with non-const parameters_, because this is not
> changing
> > > how the code worked until now (only we are clear about not keeping it
> const).
> > > 2. Then I will file a bug about making parameters_ const again, and
describe
> > all
> > > the details I know about it in there.
> > > 3. Somebody will tackle the problem from that bug. I can volunteer for
that
> > > because I would enjoy to learn another part of Chrome, but I cannot
> guarantee
> > I
> > > will do it quickly, other issues piled up on me.
> > > 
> > > I prefer the second option (with filing the bug) both because it separates
> two
> > > different issues, and this CL is already full of changes from the first
> issue.
> > > It will also help me to finish the other task which is waiting for fixing
> the
> > > const handling.
> > > 
> > > What do you think about this?
> > > 
> > > Thanks,
> > > Vaclav
> > 
> > Although it uses an asynchronous task underneath, Session::SetCookie is
> > guaranteed to be synchronous, so having the local copy is ok.
> 
> OK, I'll do that in the morning (my timezone is GMT+1).
> As it is quite clear to me what to do, are you fine if I do it, submit to CL
and
> TBR you, or do you want to see it before it goes to CL?
> Cheers,
> Vaclav

LGTM

[commit-bot: I haz the power, 2012-07-27 07:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10834004/61

[commit-bot: I haz the power, 2012-07-27 16:47:49]

Try job failure for 10834004-61 (retry) on linux_rel for steps
"interactive_ui_tests, browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-27 19:28:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10834004/61

[commit-bot: I haz the power, 2012-07-27 21:13:14]

Try job failure for 10834004-61 (previous was lost) on linux_chromeos for step
"update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-27 21:15:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10834004/61

[commit-bot: I haz the power, 2012-07-27 22:27:17]

Change committed as 148833