Don't attempt to forward declare StringPiece.

Don't attempt to forward declare StringPiece. #include the declaration, or remove the forward declaration where it was not actually used.

1) Forward declaring StringPiece is generally discouraged because it prevents callers from benefiting from automatic coersion from string/char* types.
2) A follow-up CL (http://codereview.chromium.org/8659047/) will make StringPiece a template, and thus awkward to forward declare. The very small number of places that were appropriately forward declaring it do not justify writing a 'string_piece_forward.h'.

This particular CL would be one of the rare ones where a forward declaration would be 'appropriate', but it does not warrant forward-declaring both the template and the typedef, and the number of similar cases does not justify creating a _forward.h header.

BUG=87634
R=tony@chromium.org

[erikwright (departed), 2011-12-07 15:22:00]

Hi Tony,

PTAL.

Thanks,

Erik

[tony, 2011-12-07 18:41:42]

LGTM, however I disagree with reason (1).  If callers want automatic coercion,
they should include string_piece.h (iwyu).  I would take that out of the CL
description.

[erikwright (departed), 2011-12-07 18:49:06]

On 2011/12/07 18:41:42, tony wrote:
> LGTM, however I disagree with reason (1).  If callers want automatic coercion,
> they should include string_piece.h (iwyu).  I would take that out of the CL
> description.

Thanks for the feedback.

In call-sites, there will typically be no reference to StringPiece in the code.
For example, the API is:

void Function(const StringPiece& parameter);

The call site is:

{
  ...
  Function("hello world");
  ...
}

or


{
  std::string value = "hello world";
  ...
  Function(value);
  ...
}

The purpose of the implicit constructors is to not require clients to really be
aware of the parameter type.

As it stands, this particular usage is explicitly described in string_piece.h,
so that debate should be opened outside the scope of this CL, I think.

Please confirm you are OK with me submitting the CL using the current
description.

Thanks,

Erik

[tony, 2011-12-07 19:26:56]

I'm OK with you submitting the CL with the current description, but I still
disagree with reason (1).

The header comment just says it is often appropriate, but it's not appropriate
here for that reason because these headers don't benefit from automatic coercion
to string/char* types. webkit_glue.h doesn't even use StringPiece and
test_shell.h only uses it as a return type.  In test_shell.h, the method that
returns a StringPiece is not called directly and it's callers (in src/net) don't
benefit from the automatic coercion.

[erikwright (departed), 2011-12-07 19:57:34]

On 2011/12/07 19:26:56, tony wrote:
> I'm OK with you submitting the CL with the current description, but I still
> disagree with reason (1).
> 
> The header comment just says it is often appropriate, but it's not appropriate
> here for that reason because these headers don't benefit from automatic
coercion
> to string/char* types. webkit_glue.h doesn't even use StringPiece and
> test_shell.h only uses it as a return type.  In test_shell.h, the method that
> returns a StringPiece is not called directly and it's callers (in src/net)
don't
> benefit from the automatic coercion.

OK. I understand your point. Yes, in this particular CL, forward declaration
would be OK because the coersion is not applicable.

However, the fact that this is a very unusual case is the reason why it is not
justified to create a _forward.h header to simplify the forward declaration.

FWIW, I submitted about a half dozen auto-generated CLs for different owners. I
felt the description was sufficiently applicable to all, but I will update the
description of this one to be more specific.

Thanks,

Erik

[commit-bot: I haz the power, 2011-12-07 20:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/8819022/1

[commit-bot: I haz the power, 2011-12-07 20:00:35]

Presubmit check for 8819022-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  webkit/tools/test_shell/test_shell.h

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[erikwright (departed), 2011-12-07 21:19:51]

Committed manually due to copyright exception (this is Apple copyright).

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