[sync] Componentize sync: Part 4: Fix includes in third_party/protobuf

[sync] Componentize sync: Part 4: Fix includes in third_party/protobuf

Various chrome components depend on third_party/protobuf:protobuf_lite, and in order to move to component builds, third_party/protobuf:protobuf_lite must support component builds. As of today, there are a few incorrect includes and a few missing exports in third_party that block this.

This patch corrects instances of incorrect includes of headers from third_party/protobuf.gyp:protobuf_full_do_not_use, and replaces them with correct includes from third_party/protobuf.gyp:protobuf_lite

BUG=136928
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173228

[Raghu Simha, 2012-12-13 01:54:15]

Fred, please review. Thanks.

[Raghu Simha, 2012-12-13 02:00:43]

+wez for OWNERS approval of remoting/protocol/connection_to_client.cc.

[akalin, 2012-12-13 02:04:50]

LGTM, although you should probably get someone more familiar with protobuf code
to review also (and help you upstream it).

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
third_party/protobuf/README.chromium:18: building of libproto_lite as a shared
library for Windows component builds.
component build is also on Linux/Mac at this point, I think.

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/protobuf...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/protobuf...
third_party/protobuf/protobuf.gyp:206: # On Win component builds, we must
dynamically link protobuf_lite to
it looks like the comment for the main define block is omitted when copied to
this one.  Perhaps copy it also?  Or remove this copied comment block.

[Lambros, 2012-12-13 02:39:57]

remoting lgtm

[Raghu Simha, 2012-12-13 05:20:04]

On 2012/12/13 02:04:50, akalin wrote:
> LGTM, although you should probably get someone more familiar with protobuf
code
> to review also (and help you upstream it).

+agl@ for a more thorough review of the changes in third_party/protobuf, since
he originally checked in these files. (Let me know if there's someone else I
should include in the review.)

[agl, 2012-12-13 15:41:40]

I don't know enough about components to say much, but the protobuf changes seem
reasonable.

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
third_party/protobuf/README.chromium:20: This code has been patched to support
unknown field retention in protobuf-lite.
Can you grab the changes as a .patch file and check it in + reference it here?
Alternatively, you can reference the SVN revision number, although you'll have
to commit with "rxxx" and update it once landed. (Or get lucky :)

[Raghu Simha, 2012-12-13 20:07:48]

Adam, PTAL. Thanks.

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
File third_party/protobuf/README.chromium (right):

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
third_party/protobuf/README.chromium:18: building of libproto_lite as a shared
library for Windows component builds.
On 2012/12/13 02:04:50, akalin wrote:
> component build is also on Linux/Mac at this point, I think.

Right, but the changes in protobuf.gyp are only windows specific, as they have
to do with VS projects. Mac and Linux component builds work as they are without
the change.

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/README.c...
third_party/protobuf/README.chromium:20: This code has been patched to support
unknown field retention in protobuf-lite.
On 2012/12/13 15:41:41, agl wrote:
> Can you grab the changes as a .patch file and check it in + reference it here?
> Alternatively, you can reference the SVN revision number, although you'll have
> to commit with "rxxx" and update it once landed. (Or get lucky :)

I think it might be cleaner in the long run to reference the SVN number as
you've done here. I'll update the comment in another CL.

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/protobuf...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11316370/diff/1/third_party/protobuf/protobuf...
third_party/protobuf/protobuf.gyp:206: # On Win component builds, we must
dynamically link protobuf_lite to
On 2012/12/13 02:04:50, akalin wrote:
> it looks like the comment for the main define block is omitted when copied to
> this one.  Perhaps copy it also?  Or remove this copied comment block.

Nice catch. I think it makes sense to delete the copied block since it's right
below the original. Done.

[Raghu Simha, 2012-12-14 00:13:10]

+sky for changes to content/browser/speech/proto/speech_proto.gyp

[sky, 2012-12-14 00:27:57]

LGTM

[Dirk Pranke, 2012-12-14 21:00:24]

https://codereview.chromium.org/11316370/diff/10002/third_party/protobuf/prot...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11316370/diff/10002/third_party/protobuf/prot...
third_party/protobuf/protobuf.gyp:187: #
third_party/protobuf/vsprojects/readme.txt.
If your intent is for protobuf_lite to be a DLL, you need to change the 'type'
declaration on line 131 above to '<(component)'.

We should probably discuss this change a bit more off-CL so I can be sure I
understand what you're trying to do and what the right way to do it is.

[Raghu Simha, 2012-12-14 22:10:41]

Thanks for the help, Dirk. I've updated the patch.

Adam/Dirk: Could one of you please review this one final time?

It turned out that the errors were being caused by a combination of the exports
I added and the fact that protobuf_lite was still a static library.

I have reduced the scope of this change to only fix the incorrect includes in
third_party/protobuf, which is still essential to being able to componentize
sync in a future patch.

https://codereview.chromium.org/11316370/diff/10002/third_party/protobuf/prot...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/11316370/diff/10002/third_party/protobuf/prot...
third_party/protobuf/protobuf.gyp:187: #
third_party/protobuf/vsprojects/readme.txt.
On 2012/12/14 21:00:24, Dirk Pranke wrote:
> If your intent is for protobuf_lite to be a DLL, you need to change the 'type'
> declaration on line 131 above to '<(component)'.
> 
> We should probably discuss this change a bit more off-CL so I can be sure I
> understand what you're trying to do and what the right way to do it is.

Thanks for your pointers. It turns out that these exports were causing the
multiple definition linker errors. We are (at the moment) not building
protobuf_lite as a dll, and therefore, we should not be exporting symbols within
protobuf_lite. I have undone the exports in this file.

[Raghu Simha, 2012-12-14 22:20:04]

Chatted with agl offline, and this patch is good to land now.

[Dirk Pranke, 2012-12-14 22:21:24]

On 2012/12/14 22:20:04, rsimha wrote:
> Chatted with agl offline, and this patch is good to land now.

The lack of EXPORT-modifying defines or gyp changes lgtm :).