WebUIDataSource::AddInteger (like AddBoolean/AddString)

WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest

Committed: https://crrev.com/408b01b6f8b766facfc02fcbcb2226d2e3b98be0
Cr-Commit-Position: refs/heads/master@{#423816}

[michaelpg, 2016-10-06 03:34:51]

Description was changed from

==========
WebUIDataSource::AddInteger like AddBoolean/AddString

BUG=none
R=dbeam@chromium.org,piman@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

to

==========
WebUIDataSource::AddInteger like AddBoolean/AddString

BUG=none
R=dbeam@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

[michaelpg, 2016-10-06 03:34:51]

michaelpg@chromium.org changed reviewers:
+ creis@chromium.org
- piman@chromium.org

[michaelpg, 2016-10-06 03:36:15]

Description was changed from

==========
WebUIDataSource::AddInteger like AddBoolean/AddString

BUG=none
R=dbeam@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

to

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::Value.

BUG=none
R=dbeam@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

[michaelpg, 2016-10-06 03:37:07]

PTAL. The motivation is to pass "enum" values to loadTimeData.

[michaelpg, 2016-10-06 18:24:44]

michaelpg@chromium.org changed reviewers:
+ tommycli@chromium.org

[michaelpg, 2016-10-06 18:24:44]

michaelpg@chromium.org changed required reviewers:
+ creis@chromium.org, tommycli@chromium.org

[michaelpg, 2016-10-06 18:24:45]

Tommy, could you take over for Dan? (I think dschuyler also has some context
judging by the code comments...)

[tommycli, 2016-10-06 18:29:56]

lgtm this seems harmless to me.

https://codereview.chromium.org/2391073005/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_data_source_unittest.cc (right):

https://codereview.chromium.org/2391073005/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_data_source_unittest.cc:117:
source()->AddInteger("counter", 10);
maybe add a test for negative values also?

[michaelpg, 2016-10-06 18:51:55]

Description was changed from

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::Value.

BUG=none
R=dbeam@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

to

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

[michaelpg, 2016-10-06 18:58:34]

https://codereview.chromium.org/2391073005/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_data_source_unittest.cc (right):

https://codereview.chromium.org/2391073005/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_data_source_unittest.cc:117:
source()->AddInteger("counter", 10);
On 2016/10/06 18:29:56, tommycli (OOO til Oct 21) wrote:
> maybe add a test for negative values also?

Done.

[Charlie Reis, 2016-10-06 20:47:07]

Basically looks good, with one content/public restriction below.

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
content/public/browser/web_ui_data_source.h:55: virtual void AddInteger(const
std::string& name, int32_t value) = 0;
We generally don't allow adding things to content/public unless the CL also has
a use outside content/.  Can you include the use in this CL?

http://www.chromium.org/developers/content-module/content-api
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

[michaelpg, 2016-10-06 21:32:04]

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
content/public/browser/web_ui_data_source.h:55: virtual void AddInteger(const
std::string& name, int32_t value) = 0;
On 2016/10/06 20:47:06, Charlie Reis wrote:
> We generally don't allow adding things to content/public unless the CL also
has
> a use outside content/.  Can you include the use in this CL?
> 
> http://www.chromium.org/developers/content-module/content-api
> "only expose methods in the public API that embedders need. If a method is
only
> used by other code in content, it belongs in foo_impl.h and not foo.h."

Are you asking me to land a usage with this CL, or are you just asking me where
it will be used?

The usage is in another CL:
https://codereview.chromium.org/2393703005/diff/1/chrome/browser/ui/webui/set...

I can land that CL first w/out AddInteger, and add that one change to this CL if
that's best.

[Charlie Reis, 2016-10-06 21:36:12]

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
content/public/browser/web_ui_data_source.h:55: virtual void AddInteger(const
std::string& name, int32_t value) = 0;
On 2016/10/06 21:32:04, michaelpg wrote:
> On 2016/10/06 20:47:06, Charlie Reis wrote:
> > We generally don't allow adding things to content/public unless the CL also
> has
> > a use outside content/.  Can you include the use in this CL?
> > 
> > http://www.chromium.org/developers/content-module/content-api
> > "only expose methods in the public API that embedders need. If a method is
> only
> > used by other code in content, it belongs in foo_impl.h and not foo.h."
> 
> Are you asking me to land a usage with this CL, or are you just asking me
where
> it will be used?

Yes, we need to land it with a usage.  (jam@ has insisted on this in the past.)

> 
> The usage is in another CL:
>
https://codereview.chromium.org/2393703005/diff/1/chrome/browser/ui/webui/set...
> 
> I can land that CL first w/out AddInteger, and add that one change to this CL
if
> that's best.

That sounds like a good strategy to me for keeping the CLs mostly independent. 
Thanks, and sorry for the hassle.

[michaelpg, 2016-10-06 23:58:37]

Usage added.

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2391073005/diff/40001/content/public/browser/...
content/public/browser/web_ui_data_source.h:55: virtual void AddInteger(const
std::string& name, int32_t value) = 0;
On 2016/10/06 21:36:11, Charlie Reis wrote:
> On 2016/10/06 21:32:04, michaelpg wrote:
> > On 2016/10/06 20:47:06, Charlie Reis wrote:
> > > We generally don't allow adding things to content/public unless the CL
also
> > has
> > > a use outside content/.  Can you include the use in this CL?
> > > 
> > > http://www.chromium.org/developers/content-module/content-api
> > > "only expose methods in the public API that embedders need. If a method is
> > only
> > > used by other code in content, it belongs in foo_impl.h and not foo.h."
> > 
> > Are you asking me to land a usage with this CL, or are you just asking me
> where
> > it will be used?
> 
> Yes, we need to land it with a usage.  (jam@ has insisted on this in the
past.)
> 
> > 
> > The usage is in another CL:
> >
>
https://codereview.chromium.org/2393703005/diff/1/chrome/browser/ui/webui/set...
> > 
> > I can land that CL first w/out AddInteger, and add that one change to this
CL
> if
> > that's best.
> 
> That sounds like a good strategy to me for keeping the CLs mostly independent.

> Thanks, and sorry for the hassle.

Done.

[Charlie Reis, 2016-10-07 05:17:22]

Thanks!  LGTM.

[Charlie Reis, 2016-10-07 05:17:45]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 05:18:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 06:52:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 06:52:54]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-07 07:54:07]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-10-07 07:54:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2391073005/#ps60001
(title: "DateTimeHandler usage of SetInteger")

[commit-bot: I haz the power, 2016-10-07 07:54:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 07:58:47]

Description was changed from

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

to

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

[commit-bot: I haz the power, 2016-10-07 07:58:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-07 08:00:40]

Description was changed from

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest
==========

to

==========
WebUIDataSource::AddInteger (like AddBoolean/AddString)

Convenience function to add integers to a WebUIDataSource
without using AddLocalizedStrings with a base::DictionaryValue.

Limits width to 32 bits to prevent integers larger than can
be represented in ES6 (2^53 - 1). (As before, larger numbers
could still be set via AddLocalizedStrings.)

BUG=none
R=tommycli@chromium.org,creis@chromium.org
TEST=content_unittests:WebUIDataSourceTest

Committed: https://crrev.com/408b01b6f8b766facfc02fcbcb2226d2e3b98be0
Cr-Commit-Position: refs/heads/master@{#423816}
==========

[commit-bot: I haz the power, 2016-10-07 08:00:41]

Patchset 4 (id:??) landed as
https://crrev.com/408b01b6f8b766facfc02fcbcb2226d2e3b98be0
Cr-Commit-Position: refs/heads/master@{#423816}