Importer - Change const char* foo to const char foo[]

Importer - Change const char* foo to const char foo[]

Cleanup patch to address below mentioned issue -

[*] Change const char* foo to const char foo[]

BUG=416468
Committed: https://crrev.com/511ee4b6d511d85ec82fa46c4ffcd6d119abdbd7
Cr-Commit-Position: refs/heads/master@{#297282}

[Nikhil, 2014-09-22 14:33:40]

n.bansal@samsung.com changed reviewers:
+ isherman@chromium.org

[Nikhil, 2014-09-22 14:33:40]

isherman@ - This is last of the follow-up CLs that were discussed on
ImportAutofillFormData CL. Please take a look :)

[*] Change const char* foo to const char foo[]
https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...

[*] Run clang format
https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...

[*] Should we change == to >= for data coming from IPC?
Please see :
https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...

(I'll file a bug for this)

[Nikhil, 2014-09-22 14:43:37]

Ilya, I've filed a bug for this - 416468. But status is unconfirmed and I
couldn't add you in CC list or update label to UI-Browser-Import.

Please see :
https://code.google.com/p/chromium/issues/detail?id=416468

[Ilya Sherman, 2014-09-22 20:58:27]

LGTM.  Thanks, Nikhil.

[Ilya Sherman, 2014-09-22 20:59:19]

isherman@chromium.org changed reviewers:
+ dbeam@chromium.org

[Ilya Sherman, 2014-09-22 20:59:19]

+Dan for //chrome/browser/ui/webui/options/ OWNERship.

[Nikhil, 2014-09-23 02:07:44]

isherman@ - Thanks for taking a look! Just to be clear, should I update the
condition from == to >= for the data coming through IPC? Please see :
https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...

As of now, everything seems to work fine with == condition. Please let me know
WDYT.

[Ilya Sherman, 2014-09-23 04:35:45]

On 2014/09/23 02:07:44, Nikhil wrote:
> isherman@ - Thanks for taking a look! Just to be clear, should I update the
> condition from == to >= for the data coming through IPC? Please see :
>
https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
> 
> As of now, everything seems to work fine with == condition. Please let me know
> WDYT.

Ah, sorry, I missed that question.  Yes, that would be good to fix, and let's
file a bug to track that.

The best way to think of IPC messages is to treat them as untrusted and
potentially malicious input, and to build as many safeguards against that as
possible.  That's useful both in case there are bugs of some sort, and in case
one of the processes is compromised.  The goal is to try our best to ensure that
a single misbehaving process has a hard time of wedging any other process.  In
this case, changing "==" to ">=" is safe, and mostly defends against bugs like
off-by-one errors.  It would be great to be even more defensive; but it's not
obviously worth the effort.

[Nikhil, 2014-09-23 05:15:39]

On 2014/09/23 04:35:45, Ilya Sherman wrote:
> On 2014/09/23 02:07:44, Nikhil wrote:
> > isherman@ - Thanks for taking a look! Just to be clear, should I update the
> > condition from == to >= for the data coming through IPC? Please see :
> >
>
https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
> > 
> > As of now, everything seems to work fine with == condition. Please let me
know
> > WDYT.
> 
> Ah, sorry, I missed that question.  Yes, that would be good to fix, and let's
> file a bug to track that.
> 
> The best way to think of IPC messages is to treat them as untrusted and
> potentially malicious input, and to build as many safeguards against that as
> possible.  That's useful both in case there are bugs of some sort, and in case
> one of the processes is compromised.  The goal is to try our best to ensure
that
> a single misbehaving process has a hard time of wedging any other process.  In
> this case, changing "==" to ">=" is safe, and mostly defends against bugs like
> off-by-one errors.  It would be great to be even more defensive; but it's not
> obviously worth the effort.

Great! So I'll land this CL once I get go-ahead from dbeam@. And I'll file a
separate bug for tracking changes for data coming from IPC.

Thanks for the explanation :)

[Dan Beam, 2014-09-23 05:55:35]

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/import_data_handler.cc:197: "show_bottom_bar",
if there's no functional change here (and it's just whitespace), i'd just revert
this file to the way it was.

[Nikhil, 2014-09-23 06:00:26]

dbeam@ - Thanks for taking a look! Please see my response to your comment.

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/import_data_handler.cc:197: "show_bottom_bar",
On 2014/09/23 05:55:35, Dan Beam wrote:
> if there's no functional change here (and it's just whitespace), i'd just
revert
> this file to the way it was.

Hmm, there is no functional change. But it was identified on an earlier CL to
run "git cl format" for these lines. Please see :
https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...

Please let me know WDYT.

[Dan Beam, 2014-09-23 18:41:32]

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/import_data_handler.cc:197: "show_bottom_bar",
On 2014/09/23 06:00:26, Nikhil wrote:
> On 2014/09/23 05:55:35, Dan Beam wrote:
> > if there's no functional change here (and it's just whitespace), i'd just
> revert
> > this file to the way it was.
> 
> Hmm, there is no functional change. But it was identified on an earlier CL to
> run "git cl format" for these lines. Please see :
>
https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
> 
> Please let me know WDYT.

it seems like it'd be pretty easy to run clang format on the whole codebase at
once if we really wanted to (but we haven't yet because it's not perfect).  for
instance, this pass actually adds 2 lines to the file and doesn't make the code
much easier to read :(.  it's also harder to find the original author of this
code when someone changes whitespace.

i'd still just revert

[Nikhil, 2014-09-29 06:10:44]

Sorry for the delay!

dbeam@ - Thanks for reviewing this. I've reverted the clang format change based
on your suggestion :)

isherman@ - Could you please take a look again? I got late in updating this
(sorry about that).

Thanks!

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/587403002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/import_data_handler.cc:197: "show_bottom_bar",
On 2014/09/23 18:41:32, Dan Beam wrote:
> On 2014/09/23 06:00:26, Nikhil wrote:
> > On 2014/09/23 05:55:35, Dan Beam wrote:
> > > if there's no functional change here (and it's just whitespace), i'd just
> > revert
> > > this file to the way it was.
> > 
> > Hmm, there is no functional change. But it was identified on an earlier CL
to
> > run "git cl format" for these lines. Please see :
> >
>
https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
> > 
> > Please let me know WDYT.
> 
> it seems like it'd be pretty easy to run clang format on the whole codebase at
> once if we really wanted to (but we haven't yet because it's not perfect). 
for
> instance, this pass actually adds 2 lines to the file and doesn't make the
code
> much easier to read :(.  it's also harder to find the original author of this
> code when someone changes whitespace.
> 
> i'd still just revert

Acknowledged.

[Dan Beam, 2014-09-29 18:57:46]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[Ilya Sherman, 2014-09-29 20:50:42]

Still LGTM.

[Ilya Sherman, 2014-09-29 20:50:47]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-29 20:52:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587403002/20001

[commit-bot: I haz the power, 2014-09-29 22:07:35]

Committed patchset #2 (id:20001) as de32a6dbbe2233d1b64209090e0edb0e16a67b39

[commit-bot: I haz the power, 2014-09-29 22:09:51]

Patchset 2 (id:??) landed as
https://crrev.com/511ee4b6d511d85ec82fa46c4ffcd6d119abdbd7
Cr-Commit-Position: refs/heads/master@{#297282}