Pull new version of protobuf sources.

Pull new version of protobuf sources.

This should fix some MSVC warnings (though, sadly, we now have to disable more
warnings than before... different ones).  More importantly, it's the first full
source update (as opposed to cherry-picks) done in years, and includes a
complete README and sequence of patches to help future maintainers do similar
updates.  (Several past changes just updated the README to mention a change
without supplying a patch or even pointing to the relevant Chrome revision.)  My
hope is that this will make future source updates -- and maybe eventually
pulling directly from upstream via DEPS -- more feasible, so we don't get into a
similar state of having our local tree diverge so badly that only cherry-picks
are really doable.

BUG=81439
TEST=none
TBR=scottmg

Committed: https://crrev.com/abf57c3926331ce8f98f831c526581a038172445
Cr-Commit-Position: refs/heads/master@{#345578}

[Peter Kasting, 2015-08-13 00:58:24]

pkasting@chromium.org changed reviewers:
+ atwilson@chromium.org, scottmg@chromium.org, thakis@chromium.org

[Peter Kasting, 2015-08-13 00:58:29]

Notes on reviewers and what to look for:

+scottmg as OWNER.  Obviously you can't really review most of this, so focus on
DEPS, OWNERS, README.chromium, and the .gyp* and .gn* files.  Perhaps you also
know if there's a pressing reason why I should *not* have updated to whatever
the current upstream master contains (e.g. protobuf is currently making some
breaking behavioral change).

+thakis as someone who's patched our previous set of sources a number of times
and might be aware of landmines I should have looked out for.  Nico, one thing
you might want to do after I land this (or before if you get me to send you a
.zip of the new directory, I think Rietveld won't let you D/L this patch) is to
check whether your patch to deinline various functions to save space is still
the right thing to do.  It's still applied (see patches/04_uninline.patch) but
since upstream has changed a lot I don't know if it still makes sense.

+atwilson as a sync OWNER.  sync is the main thing in Chrome that makes use of
the "preserve unknown fields" capability that was added years ago (see
patches/02_unknown_fields_in_protobuf_lite.patch).  This was by far the largest
and most complicated patch to port to the new source base and I'm not at all
sure I got it right.  You sync folks may know of a way to test that Chromium's
support for this is still working correctly, which would make me feel a lot less
nervous about possibly breaking Chrome's in-the-wild behavior in strange ways.

[Nico, 2015-08-13 01:10:09]

Cool!

If I remember correctly, the reason for moving this from DEPS to our repo was
that agl wanted to change how unknown fields are handled. It looks like you have
a patch for this, so that part looks good from a distance.

On 2015/08/13 00:58:29, Peter Kasting wrote:
> Notes on reviewers and what to look for:
> 
> +scottmg as OWNER.  Obviously you can't really review most of this, so focus
on
> DEPS, OWNERS, README.chromium, and the .gyp* and .gn* files.  Perhaps you also
> know if there's a pressing reason why I should *not* have updated to whatever
> the current upstream master contains (e.g. protobuf is currently making some
> breaking behavioral change).
> 
> +thakis as someone who's patched our previous set of sources a number of times
> and might be aware of landmines I should have looked out for.  Nico, one thing
> you might want to do after I land this (or before if you get me to send you a
> .zip of the new directory, I think Rietveld won't let you D/L this patch) is
to
> check whether your patch to deinline various functions to save space

Hm, I think this might've been dcheng's patch (
https://codereview.chromium.org/778203003 ), not mine?

> is still
> the right thing to do.  It's still applied (see patches/04_uninline.patch) but
> since upstream has changed a lot I don't know if it still makes sense.
> 
> +atwilson as a sync OWNER.  sync is the main thing in Chrome that makes use of
> the "preserve unknown fields" capability that was added years ago (see
> patches/02_unknown_fields_in_protobuf_lite.patch).  This was by far the
largest
> and most complicated patch to port to the new source base and I'm not at all
> sure I got it right.  You sync folks may know of a way to test that Chromium's
> support for this is still working correctly, which would make me feel a lot
less
> nervous about possibly breaking Chrome's in-the-wild behavior in strange ways.

[scottmg, 2015-08-13 02:55:56]

scottmg@chromium.org changed reviewers:
+ dcheng@chromium.org

[scottmg, 2015-08-13 02:56:02]

README and build files look ok.

I shouldn't really be owner here, I've only ever fiddled with the gyp files and
fixed a few warnings. (You're probably a better OWNERS now that you've gone
through this. If you'd like to replace me with you either here, or in a
followup, please do.)

The unknown fields patch certainly sees scary. I would second the request for a
solid test (hopefully we have a test the does an "upgrade" thing?) or maybe you
could ask agl@ for a suggestion of what to try/test?

I think you should keep the deinlining patch, +dcheng.

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
File third_party/protobuf/descriptor_pb2.py (left):

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
third_party/protobuf/descriptor_pb2.py:1: # Generated by the protocol buffer
compiler.  DO NOT EDIT!
Isn't this supposed to be regenerated? Why is it a D?

[Peter Kasting, 2015-08-13 05:21:47]

On 2015/08/13 01:10:09, Nico (hiding) wrote:
> Cool!
> 
> If I remember correctly, the reason for moving this from DEPS to our repo was
> that agl wanted to change how unknown fields are handled. It looks like you
have
> a patch for this, so that part looks good from a distance.

I wonder if the upstream maintainers would be interested in taking this; it
certainly took up most of the two weeks it took me to make this upgrade happen.

> On 2015/08/13 00:58:29, Peter Kasting wrote:
> > +thakis as someone who's patched our previous set of sources a number of
times
> > and might be aware of landmines I should have looked out for.  Nico, one
thing
> > you might want to do after I land this (or before if you get me to send you
a
> > .zip of the new directory, I think Rietveld won't let you D/L this patch) is
> to
> > check whether your patch to deinline various functions to save space
> 
> Hm, I think this might've been dcheng's patch (
> https://codereview.chromium.org/778203003 ), not mine?

Oops, yeah.  Why was I thinking you did this?  Durr.

[Nico, 2015-08-13 05:25:01]

On Wed, Aug 12, 2015 at 10:21 PM, <pkasting@chromium.org> wrote:

> On 2015/08/13 01:10:09, Nico (hiding) wrote:
>
>> Cool!
>>
>
> If I remember correctly, the reason for moving this from DEPS to our repo
>> was
>> that agl wanted to change how unknown fields are handled. It looks like
>> you
>>
> have
>
>> a patch for this, so that part looks good from a distance.
>>
>
> I wonder if the upstream maintainers would be interested in taking this; it
> certainly took up most of the two weeks it took me to make this upgrade
> happen.
>

Back when agl did it, they weren't. It's different maintainers now though I
think; maybe we should try again.


>
> On 2015/08/13 00:58:29, Peter Kasting wrote:
>> > +thakis as someone who's patched our previous set of sources a number of
>>
> times
>
>> > and might be aware of landmines I should have looked out for.  Nico, one
>>
> thing
>
>> > you might want to do after I land this (or before if you get me to send
>> you
>>
> a
>
>> > .zip of the new directory, I think Rietveld won't let you D/L this
>> patch) is
>> to
>> > check whether your patch to deinline various functions to save space
>>
>
> Hm, I think this might've been dcheng's patch (
>> https://codereview.chromium.org/778203003 ), not mine?
>>
>
> Oops, yeah.  Why was I thinking you did this?  Durr.
>
> https://codereview.chromium.org/1291903002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-08-13 05:33:31]

On 2015/08/13 at 05:21:47, pkasting wrote:
> On 2015/08/13 01:10:09, Nico (hiding) wrote:
> > Cool!
> > 
> > If I remember correctly, the reason for moving this from DEPS to our repo
was
> > that agl wanted to change how unknown fields are handled. It looks like you
have
> > a patch for this, so that part looks good from a distance.
> 
> I wonder if the upstream maintainers would be interested in taking this; it
certainly took up most of the two weeks it took me to make this upgrade happen.
> 
> > On 2015/08/13 00:58:29, Peter Kasting wrote:
> > > +thakis as someone who's patched our previous set of sources a number of
times
> > > and might be aware of landmines I should have looked out for.  Nico, one
thing
> > > you might want to do after I land this (or before if you get me to send
you a
> > > .zip of the new directory, I think Rietveld won't let you D/L this patch)
is
> > to
> > > check whether your patch to deinline various functions to save space
> > 
> > Hm, I think this might've been dcheng's patch (
> > https://codereview.chromium.org/778203003 ), not mine?
> 
> Oops, yeah.  Why was I thinking you did this?  Durr.

It's actually possible the uninlining patch may no longer be needed.
https://github.com/google/protobuf/issues/125#issuecomment-66724317 says the
protoc generator greatly reduced the usage of GetEmptyString() to one per
generated .pb.cc, and no size improvements were observed with my patch after
v2.6.0 (the version we're pulling looks to be v3.0.0)

[Peter Kasting, 2015-08-13 05:43:41]

On 2015/08/13 05:33:31, dcheng wrote:
> It's actually possible the uninlining patch may no longer be needed.
> https://github.com/google/protobuf/issues/125#issuecomment-66724317 says the
> protoc generator greatly reduced the usage of GetEmptyString() to one per
> generated .pb.cc, and no size improvements were observed with my patch after
> v2.6.0 (the version we're pulling looks to be v3.0.0)

This is a release build of chrome.dll, right?  I can check the sizes of this on
Windows with and without your patch.  Would that be good enough to tell the
difference?

[Peter Kasting, 2015-08-13 05:44:05]

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
File third_party/protobuf/descriptor_pb2.py (left):

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
third_party/protobuf/descriptor_pb2.py:1: # Generated by the protocol buffer
compiler.  DO NOT EDIT!
On 2015/08/13 02:56:02, scottmg wrote:
> Isn't this supposed to be regenerated? Why is it a D?

I put the new generated version in python/google/protobuf/, which is more where
it's "supposed" to live.  It still is manually generated, though.

[dcheng, 2015-08-13 05:54:30]

On 2015/08/13 at 05:43:41, pkasting wrote:
> On 2015/08/13 05:33:31, dcheng wrote:
> > It's actually possible the uninlining patch may no longer be needed.
> > https://github.com/google/protobuf/issues/125#issuecomment-66724317 says the
> > protoc generator greatly reduced the usage of GetEmptyString() to one per
> > generated .pb.cc, and no size improvements were observed with my patch after
> > v2.6.0 (the version we're pulling looks to be v3.0.0)
> 
> This is a release build of chrome.dll, right?  I can check the sizes of this
on Windows with and without your patch.  Would that be good enough to tell the
difference?

The most noticeable difference was on Linux. I'll try patching this in tomorrow
to see if it makes a difference.

[Peter Kasting, 2015-08-13 07:57:37]

On 2015/08/13 05:54:30, dcheng wrote:
> On 2015/08/13 at 05:43:41, pkasting wrote:
> > On 2015/08/13 05:33:31, dcheng wrote:
> > > It's actually possible the uninlining patch may no longer be needed.
> > > https://github.com/google/protobuf/issues/125#issuecomment-66724317 says
the
> > > protoc generator greatly reduced the usage of GetEmptyString() to one per
> > > generated .pb.cc, and no size improvements were observed with my patch
after
> > > v2.6.0 (the version we're pulling looks to be v3.0.0)
> > 
> > This is a release build of chrome.dll, right?  I can check the sizes of this
> on Windows with and without your patch.  Would that be good enough to tell the
> difference?
> 
> The most noticeable difference was on Linux. I'll try patching this in
tomorrow
> to see if it makes a difference.

Let me know if you need an archive of the directory -- trying to download a
patch from here may not work very well :/

[Andrew T Wilson (Slow), 2015-08-13 10:40:47]

atwilson@chromium.org changed reviewers:
+ zea@chromium.org
- atwilson@chromium.org

[Andrew T Wilson (Slow), 2015-08-13 10:42:03]

+zea to look at this from a sync perspective, since I'm not really familiar with
this area of the code.

[scottmg, 2015-08-13 16:25:03]

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
File third_party/protobuf/descriptor_pb2.py (left):

https://codereview.chromium.org/1291903002/diff/1/third_party/protobuf/descri...
third_party/protobuf/descriptor_pb2.py:1: # Generated by the protocol buffer
compiler.  DO NOT EDIT!
On 2015/08/13 05:44:04, Peter Kasting wrote:
> On 2015/08/13 02:56:02, scottmg wrote:
> > Isn't this supposed to be regenerated? Why is it a D?
> 
> I put the new generated version in python/google/protobuf/, which is more
where
> it's "supposed" to live.  It still is manually generated, though.

Sorry, I don't see it as "python/google/protobuf/descriptor_pb2.py" which is
what README.chromium says. Am I looking for the wrong thing?

[Peter Kasting, 2015-08-13 21:18:41]

New snap up:
* Changed OWNERS from scottmg to myself
* Added descriptor_pb2.py correctly, it was indeed missing
* Added note to README about autogenerating files for the changes in the big
unknown fields patch
* Fixed some bugs exposed by autogenerating those files

I haven't removed the inlining patch, still waiting for dcheng's word on that.

[Nicolas Zea, 2015-08-18 16:06:11]

On 2015/08/13 21:18:41, Peter Kasting wrote:
> New snap up:
> * Changed OWNERS from scottmg to myself
> * Added descriptor_pb2.py correctly, it was indeed missing
> * Added note to README about autogenerating files for the changes in the big
> unknown fields patch
> * Fixed some bugs exposed by autogenerating those files
> 
> I haven't removed the inlining patch, still waiting for dcheng's word on that.

If sync_unit_tests is passing (which runs by default on trybots), then Sync
should be fine. We have unit tests to ensure that unknown fields are preserved.

[Peter Kasting, 2015-08-26 05:50:50]

This seems to be OK and AFAICT there are no objections, so TBRing to scottmg so
I can CQ+ this.

[Peter Kasting, 2015-08-26 05:50:54]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-08-26 05:51:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291903002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291903002/110001

[commit-bot: I haz the power, 2015-08-26 09:01:08]

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

[Peter Kasting, 2015-08-26 09:03:14]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-08-26 09:03:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291903002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291903002/110001

[commit-bot: I haz the power, 2015-08-26 10:35:33]

Committed patchset #12 (id:110001)

[commit-bot: I haz the power, 2015-08-26 10:36:21]

Patchset 12 (id:??) landed as
https://crrev.com/abf57c3926331ce8f98f831c526581a038172445
Cr-Commit-Position: refs/heads/master@{#345578}

[xyzzyz, 2016-04-14 00:04:49]

xyzzyz@chromium.org changed reviewers:
+ xyzzyz@chromium.org

[chromium-reviews, 2016-04-14 00:05:30]

Oops, wrong issue, ignore this.

On Wed, Apr 13, 2016 at 5:04 PM <xyzzyz@chromium.org> wrote:

> https://codereview.chromium.org/1291903002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.