Images are binaries too, yo.

Images are binaries too, yo.

Make upload.py treat images as binaries, so they upload correctly and display in Reitveld.

BUG=227346
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=198833

[Julie Parent, 2013-05-06 22:57:15]

I am not familiar with this code, so I don't know all the consequences of a
change like this.  I have tested with uploading a new image, a changed image,
and a changed txt file.  See https://codereview.chromium.org/15018004/ for
example using these changes.

[Dirk Pranke, 2013-05-06 23:00:51]

Looks plausible to me, but I'm not a reviewer for Rietveld changes. Robbie,
Marc-Antoine, WDYT?

[iannucci, 2013-05-06 23:15:23]

On 2013/05/06 23:00:51, Dirk Pranke wrote:
> Looks plausible to me, but I'm not a reviewer for Rietveld changes. Robbie,
> Marc-Antoine, WDYT?

Not really sure about this... AFAIK, rietveld already correctly handles images
(ex. https://codereview.appspot.com/7388060/diff/68001/static/automated.png)

What is the error that you're running into?

[Julie Parent, 2013-05-06 23:17:40]

See the attached bug for more on the issue (all pngs were being uploaded and
displayed as txt files)

On 2013/05/06 23:15:23, iannucci wrote:
> On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > Looks plausible to me, but I'm not a reviewer for Rietveld changes. Robbie,
> > Marc-Antoine, WDYT?
> 
> Not really sure about this... AFAIK, rietveld already correctly handles images
> (ex. https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> 
> What is the error that you're running into?

[iannucci, 2013-05-06 23:22:52]

On 2013/05/06 23:17:40, Julie Parent wrote:
> See the attached bug for more on the issue (all pngs were being uploaded and
> displayed as txt files)
> 
> On 2013/05/06 23:15:23, iannucci wrote:
> > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > > Looks plausible to me, but I'm not a reviewer for Rietveld changes.
Robbie,
> > > Marc-Antoine, WDYT?
> > 
> > Not really sure about this... AFAIK, rietveld already correctly handles
images
> > (ex. https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> > 
> > What is the error that you're running into?

Ah, ok. This is probably a bug that recently snuck in with the latest merge of
upload.py.

If you've tested this and it works, could you apply the patch to upstream
rietveld (the default branch of
https://code.google.com/p/rietveld/source/checkout . Make your patch and run
./upload.py to have it post to codereview.appspot.com.)  Then we can land it
upstream and I'll merge to the chromium branch of rietveld, and finally you or I
can land the upstreamed version back in depot_tools.

[Julie Parent, 2013-05-06 23:27:24]

Hmm, my understanding was that this had never worked. Can someone more familiar
with this comment?

On 2013/05/06 23:22:52, iannucci wrote:
> On 2013/05/06 23:17:40, Julie Parent wrote:
> > See the attached bug for more on the issue (all pngs were being uploaded and
> > displayed as txt files)
> > 
> > On 2013/05/06 23:15:23, iannucci wrote:
> > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > > > Looks plausible to me, but I'm not a reviewer for Rietveld changes.
> Robbie,
> > > > Marc-Antoine, WDYT?
> > > 
> > > Not really sure about this... AFAIK, rietveld already correctly handles
> images
> > > (ex.
https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> > > 
> > > What is the error that you're running into?
> 
> Ah, ok. This is probably a bug that recently snuck in with the latest merge of
> upload.py.
> 
> If you've tested this and it works, could you apply the patch to upstream
> rietveld (the default branch of
> https://code.google.com/p/rietveld/source/checkout . Make your patch and run
> ./upload.py to have it post to http://codereview.appspot.com.)  Then we can
land it
> upstream and I'll merge to the chromium branch of rietveld, and finally you or
I
> can land the upstreamed version back in depot_tools.

[iannucci, 2013-05-06 23:33:21]

So, there are actually a couple things at work. Rietveld (i.e. the thing running
on codereview.appspot.com) does support binary files. The Chromium flavor of
Rietveld (running the page you see here) doesn't /quite/ support them (i.e.
there are a cascading series of changes in order to support them).

If there's an upstream issue where Rietveld's upload of binaries failed, then
that's fairly recent and may, in fact, be fixable with a one-line change. (I
know because I witnessed it working as recently as 3 weeks ago).

If you're trying to fix the fact that chromium's rietveld doesn't work with
binaries, there are actually a series of systems (cq, tryserver, rietveld) which
all need to be correctly fixed to support binaries properly.

Does that make more sense? It's a pretty suboptimal state to be in, but that's
where we're at currently :/

Rob

On 2013/05/06 23:27:24, Julie Parent wrote:
> Hmm, my understanding was that this had never worked. Can someone more
familiar
> with this comment?
> 
> On 2013/05/06 23:22:52, iannucci wrote:
> > On 2013/05/06 23:17:40, Julie Parent wrote:
> > > See the attached bug for more on the issue (all pngs were being uploaded
and
> > > displayed as txt files)
> > > 
> > > On 2013/05/06 23:15:23, iannucci wrote:
> > > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > > > > Looks plausible to me, but I'm not a reviewer for Rietveld changes.
> > Robbie,
> > > > > Marc-Antoine, WDYT?
> > > > 
> > > > Not really sure about this... AFAIK, rietveld already correctly handles
> > images
> > > > (ex.
> https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> > > > 
> > > > What is the error that you're running into?
> > 
> > Ah, ok. This is probably a bug that recently snuck in with the latest merge
of
> > upload.py.
> > 
> > If you've tested this and it works, could you apply the patch to upstream
> > rietveld (the default branch of
> > https://code.google.com/p/rietveld/source/checkout . Make your patch and run
> > ./upload.py to have it post to http://codereview.appspot.com.)  Then we can
> land it
> > upstream and I'll merge to the chromium branch of rietveld, and finally you
or
> I
> > can land the upstreamed version back in depot_tools.

[jam, 2013-05-06 23:42:57]

On 2013/05/06 23:33:21, iannucci wrote:
> So, there are actually a couple things at work. Rietveld (i.e. the thing
running
> on http://codereview.appspot.com) does support binary files. The Chromium
flavor of
> Rietveld (running the page you see here) doesn't /quite/ support them (i.e.
> there are a cascading series of changes in order to support them).

I'm not sure I follow what you mean.

Rietveld and our instance used to support uploading images (and diffing
old/new). I know this because I added it 4.5 years ago in
https://codereview.appspot.com/3270. If this broke, that's a regression.
> 
> If there's an upstream issue where Rietveld's upload of binaries failed, then
> that's fairly recent and may, in fact, be fixable with a one-line change. (I
> know because I witnessed it working as recently as 3 weeks ago).
> 
> If you're trying to fix the fact that chromium's rietveld doesn't work with
> binaries, there are actually a series of systems (cq, tryserver, rietveld)
which
> all need to be correctly fixed to support binaries properly.
> 
> Does that make more sense? It's a pretty suboptimal state to be in, but that's
> where we're at currently :/
> 
> Rob
> 
> On 2013/05/06 23:27:24, Julie Parent wrote:
> > Hmm, my understanding was that this had never worked. Can someone more
> familiar
> > with this comment?
> > 
> > On 2013/05/06 23:22:52, iannucci wrote:
> > > On 2013/05/06 23:17:40, Julie Parent wrote:
> > > > See the attached bug for more on the issue (all pngs were being uploaded
> and
> > > > displayed as txt files)
> > > > 
> > > > On 2013/05/06 23:15:23, iannucci wrote:
> > > > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > > > > > Looks plausible to me, but I'm not a reviewer for Rietveld changes.
> > > Robbie,
> > > > > > Marc-Antoine, WDYT?
> > > > > 
> > > > > Not really sure about this... AFAIK, rietveld already correctly
handles
> > > images
> > > > > (ex.
> > https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> > > > > 
> > > > > What is the error that you're running into?
> > > 
> > > Ah, ok. This is probably a bug that recently snuck in with the latest
merge
> of
> > > upload.py.
> > > 
> > > If you've tested this and it works, could you apply the patch to upstream
> > > rietveld (the default branch of
> > > https://code.google.com/p/rietveld/source/checkout . Make your patch and
run
> > > ./upload.py to have it post to http://codereview.appspot.com.)  Then we
can
> > land it
> > > upstream and I'll merge to the chromium branch of rietveld, and finally
you
> or
> > I
> > > can land the upstreamed version back in depot_tools.

[iannucci, 2013-05-06 23:52:13]

On 2013/05/06 23:42:57, jam wrote:
> On 2013/05/06 23:33:21, iannucci wrote:
> > So, there are actually a couple things at work. Rietveld (i.e. the thing
> running
> > on http://codereview.appspot.com) does support binary files. The Chromium
> flavor of
> > Rietveld (running the page you see here) doesn't /quite/ support them (i.e.
> > there are a cascading series of changes in order to support them).
> 
> I'm not sure I follow what you mean.
> 
> Rietveld and our instance used to support uploading images (and diffing
> old/new). I know this because I added it 4.5 years ago in
> https://codereview.appspot.com/3270. If this broke, that's a regression.

It sounds like you follow exactly what I mean :).

There currently exists a regression between rietveld:default and
rietveld:chromium such that binary support is working on default but not working
on chromium (mod a possible recent bug on default). AFAIK it's off on chromium
because there are a number of systems (cq and trybots) which don't work
correctly with binaries.

> > 
> > If there's an upstream issue where Rietveld's upload of binaries failed,
then
> > that's fairly recent and may, in fact, be fixable with a one-line change. (I
> > know because I witnessed it working as recently as 3 weeks ago).
> > 
> > If you're trying to fix the fact that chromium's rietveld doesn't work with
> > binaries, there are actually a series of systems (cq, tryserver, rietveld)
> which
> > all need to be correctly fixed to support binaries properly.
> > 
> > Does that make more sense? It's a pretty suboptimal state to be in, but
that's
> > where we're at currently :/
> > 
> > Rob
> > 
> > On 2013/05/06 23:27:24, Julie Parent wrote:
> > > Hmm, my understanding was that this had never worked. Can someone more
> > familiar
> > > with this comment?
> > > 
> > > On 2013/05/06 23:22:52, iannucci wrote:
> > > > On 2013/05/06 23:17:40, Julie Parent wrote:
> > > > > See the attached bug for more on the issue (all pngs were being
uploaded
> > and
> > > > > displayed as txt files)
> > > > > 
> > > > > On 2013/05/06 23:15:23, iannucci wrote:
> > > > > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> > > > > > > Looks plausible to me, but I'm not a reviewer for Rietveld
changes.
> > > > Robbie,
> > > > > > > Marc-Antoine, WDYT?
> > > > > > 
> > > > > > Not really sure about this... AFAIK, rietveld already correctly
> handles
> > > > images
> > > > > > (ex.
> > > https://codereview.appspot.com/7388060/diff/68001/static/automated.png)
> > > > > > 
> > > > > > What is the error that you're running into?
> > > > 
> > > > Ah, ok. This is probably a bug that recently snuck in with the latest
> merge
> > of
> > > > upload.py.
> > > > 
> > > > If you've tested this and it works, could you apply the patch to
upstream
> > > > rietveld (the default branch of
> > > > https://code.google.com/p/rietveld/source/checkout . Make your patch and
> run
> > > > ./upload.py to have it post to http://codereview.appspot.com.)  Then we
> can
> > > land it
> > > > upstream and I'll merge to the chromium branch of rietveld, and finally
> you
> > or
> > > I
> > > > can land the upstreamed version back in depot_tools.

[iannucci, 2013-05-06 23:58:45]

(that said, I dont see why we can't land this patch in upstream).

[jam, 2013-05-06 23:59:03]

On Mon, May 6, 2013 at 4:52 PM, <iannucci@chromium.org> wrote:

> On 2013/05/06 23:42:57, jam wrote:
>
>> On 2013/05/06 23:33:21, iannucci wrote:
>> > So, there are actually a couple things at work. Rietveld (i.e. the thing
>> running
>> > on http://codereview.appspot.com) does support binary files. The
>> Chromium
>> flavor of
>> > Rietveld (running the page you see here) doesn't /quite/ support them
>> (i.e.
>> > there are a cascading series of changes in order to support them).
>>
>
>  I'm not sure I follow what you mean.
>>
>
>  Rietveld and our instance used to support uploading images (and diffing
>> old/new). I know this because I added it 4.5 years ago in
>> https://codereview.appspot.**com/3270<https://codereview.appspot.com/3270>.
>> If this broke, that's a regression.
>>
>
> It sounds like you follow exactly what I mean :).
>
> There currently exists a regression between rietveld:default and
> rietveld:chromium such that binary support is working on default but not
> working
> on chromium (mod a possible recent bug on default). AFAIK it's off on
> chromium
> because there are a number of systems (cq and trybots) which don't work
> correctly with binaries.


ok with this more information, I can tell you that this last sentence is
still confusing me.

Just because trybots and CQ don't support binaries, how does that affect
the upload to Rietveld? i.e. the former gets started by "gcl try" while the
latter is "gcl upload".


>
>  >
>> > If there's an upstream issue where Rietveld's upload of binaries failed,
>>
> then
>
>> > that's fairly recent and may, in fact, be fixable with a one-line
>> change. (I
>> > know because I witnessed it working as recently as 3 weeks ago).
>> >
>> > If you're trying to fix the fact that chromium's rietveld doesn't work
>> with
>> > binaries, there are actually a series of systems (cq, tryserver,
>> rietveld)
>> which
>> > all need to be correctly fixed to support binaries properly.
>> >
>> > Does that make more sense? It's a pretty suboptimal state to be in, but
>>
> that's
>
>> > where we're at currently :/
>> >
>> > Rob
>> >
>> > On 2013/05/06 23:27:24, Julie Parent wrote:
>> > > Hmm, my understanding was that this had never worked. Can someone more
>> > familiar
>> > > with this comment?
>> > >
>> > > On 2013/05/06 23:22:52, iannucci wrote:
>> > > > On 2013/05/06 23:17:40, Julie Parent wrote:
>> > > > > See the attached bug for more on the issue (all pngs were being
>>
> uploaded
>
>> > and
>> > > > > displayed as txt files)
>> > > > >
>> > > > > On 2013/05/06 23:15:23, iannucci wrote:
>> > > > > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
>> > > > > > > Looks plausible to me, but I'm not a reviewer for Rietveld
>>
> changes.
>
>> > > > Robbie,
>> > > > > > > Marc-Antoine, WDYT?
>> > > > > >
>> > > > > > Not really sure about this... AFAIK, rietveld already correctly
>> handles
>> > > > images
>> > > > > > (ex.
>> > > https://codereview.appspot.**com/7388060/diff/68001/static/**
>>
automated.png<https://codereview.appspot.com/7388060/diff/68001/static/automated.png>
>> )
>> > > > > >
>> > > > > > What is the error that you're running into?
>> > > >
>> > > > Ah, ok. This is probably a bug that recently snuck in with the
>> latest
>> merge
>> > of
>> > > > upload.py.
>> > > >
>> > > > If you've tested this and it works, could you apply the patch to
>>
> upstream
>
>> > > > rietveld (the default branch of
>> > > >
https://code.google.com/p/**rietveld/source/checkout<https://code.google.com/....
Make your patch and
>> run
>> > > > ./upload.py to have it post to http://codereview.appspot.com.**)
>>  Then we
>> can
>> > > land it
>> > > > upstream and I'll merge to the chromium branch of rietveld, and
>> finally
>> you
>> > or
>> > > I
>> > > > can land the upstreamed version back in depot_tools.
>>
>
>
>
>
https://codereview.chromium.**org/14820015/<https://codereview.chromium.org/1...
>

[iannucci, 2013-05-07 00:02:01]

On 2013/05/06 23:58:45, iannucci wrote:
> (that said, I dont see why we can't land this patch in upstream).

(( also, I'm looking at the rietveld code and I think I'm wrong... chromium
rietveld isn't currently blocking binary upload, though the CQ and TS still
appear not to support it)).

[Julie Parent, 2013-05-07 00:03:06]

https://codereview.appspot.com/6221063/ enables binary uploads for NOT images,
but that changes is almost a year old.  I'm not sure where a more recent
regression would come from, looking at the history of upload.py.

On 2013/05/06 23:59:03, jam wrote:
> On Mon, May 6, 2013 at 4:52 PM, <mailto:iannucci@chromium.org> wrote:
> 
> > On 2013/05/06 23:42:57, jam wrote:
> >
> >> On 2013/05/06 23:33:21, iannucci wrote:
> >> > So, there are actually a couple things at work. Rietveld (i.e. the thing
> >> running
> >> > on http://codereview.appspot.com) does support binary files. The
> >> Chromium
> >> flavor of
> >> > Rietveld (running the page you see here) doesn't /quite/ support them
> >> (i.e.
> >> > there are a cascading series of changes in order to support them).
> >>
> >
> >  I'm not sure I follow what you mean.
> >>
> >
> >  Rietveld and our instance used to support uploading images (and diffing
> >> old/new). I know this because I added it 4.5 years ago in
> >>
https://codereview.appspot.**com/3270%3Chttps://codereview.appspot.com/3270>.
> >> If this broke, that's a regression.
> >>
> >
> > It sounds like you follow exactly what I mean :).
> >
> > There currently exists a regression between rietveld:default and
> > rietveld:chromium such that binary support is working on default but not
> > working
> > on chromium (mod a possible recent bug on default). AFAIK it's off on
> > chromium
> > because there are a number of systems (cq and trybots) which don't work
> > correctly with binaries.
> 
> 
> ok with this more information, I can tell you that this last sentence is
> still confusing me.
> 
> Just because trybots and CQ don't support binaries, how does that affect
> the upload to Rietveld? i.e. the former gets started by "gcl try" while the
> latter is "gcl upload".
> 
> 
> >
> >  >
> >> > If there's an upstream issue where Rietveld's upload of binaries failed,
> >>
> > then
> >
> >> > that's fairly recent and may, in fact, be fixable with a one-line
> >> change. (I
> >> > know because I witnessed it working as recently as 3 weeks ago).
> >> >
> >> > If you're trying to fix the fact that chromium's rietveld doesn't work
> >> with
> >> > binaries, there are actually a series of systems (cq, tryserver,
> >> rietveld)
> >> which
> >> > all need to be correctly fixed to support binaries properly.
> >> >
> >> > Does that make more sense? It's a pretty suboptimal state to be in, but
> >>
> > that's
> >
> >> > where we're at currently :/
> >> >
> >> > Rob
> >> >
> >> > On 2013/05/06 23:27:24, Julie Parent wrote:
> >> > > Hmm, my understanding was that this had never worked. Can someone more
> >> > familiar
> >> > > with this comment?
> >> > >
> >> > > On 2013/05/06 23:22:52, iannucci wrote:
> >> > > > On 2013/05/06 23:17:40, Julie Parent wrote:
> >> > > > > See the attached bug for more on the issue (all pngs were being
> >>
> > uploaded
> >
> >> > and
> >> > > > > displayed as txt files)
> >> > > > >
> >> > > > > On 2013/05/06 23:15:23, iannucci wrote:
> >> > > > > > On 2013/05/06 23:00:51, Dirk Pranke wrote:
> >> > > > > > > Looks plausible to me, but I'm not a reviewer for Rietveld
> >>
> > changes.
> >
> >> > > > Robbie,
> >> > > > > > > Marc-Antoine, WDYT?
> >> > > > > >
> >> > > > > > Not really sure about this... AFAIK, rietveld already correctly
> >> handles
> >> > > > images
> >> > > > > > (ex.
> >> > > https://codereview.appspot.**com/7388060/diff/68001/static/**
> >>
>
automated.png<https://codereview.appspot.com/7388060/diff/68001/static/automated.png>
> >> )
> >> > > > > >
> >> > > > > > What is the error that you're running into?
> >> > > >
> >> > > > Ah, ok. This is probably a bug that recently snuck in with the
> >> latest
> >> merge
> >> > of
> >> > > > upload.py.
> >> > > >
> >> > > > If you've tested this and it works, could you apply the patch to
> >>
> > upstream
> >
> >> > > > rietveld (the default branch of
> >> > > >
>
https://code.google.com/p/**rietveld/source/checkout%3Chttps://code.google.co...>.
> Make your patch and
> >> run
> >> > > > ./upload.py to have it post to http://codereview.appspot.com.**)
> >>  Then we
> >> can
> >> > > land it
> >> > > > upstream and I'll merge to the chromium branch of rietveld, and
> >> finally
> >> you
> >> > or
> >> > > I
> >> > > > can land the upstreamed version back in depot_tools.
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/14820015/%3Chttps://codereview.chromium.org...>
> >

[M-A Ruel, 2013-05-07 00:06:23]

On 2013/05/07 00:02:01, iannucci wrote:
> On 2013/05/06 23:58:45, iannucci wrote:
> > (that said, I dont see why we can't land this patch in upstream).
> 
> (( also, I'm looking at the rietveld code and I think I'm wrong... chromium
> rietveld isn't currently blocking binary upload, though the CQ and TS still
> appear not to support it)).

Rietveld is not blocking anything. To block the CQ from committing broken files,
depot_tools/rietveld.py naively tries to block anything that looks like binary
because there has been issues in properly handling binary files. Issues looked
like corrupted file and 0-length file. The code is in Rietveld.get_patch() and
the exact same code is used by both apply_issue.py and the CQ.

[iannucci, 2013-05-07 00:09:07]

So, I think I was just flat-out wrong here :/. I thought that there was actually
a change on the server-side which blocked uploading of binaries to chromium
rietveld.

It is true that cq/ts don't (currently) work with binary patches. For some
reason, I thought that there was a preventative block in rietveld itself to
prevent these from getting uploaded at all. It's good that there's not.

Sorry for the kerfluffle...

In any event, this patch should be landed upstream first, regardless :)

[iannucci, 2013-05-07 00:09:58]

On 2013/05/07 00:06:23, Marc-Antoine Ruel wrote:
> On 2013/05/07 00:02:01, iannucci wrote:
> > On 2013/05/06 23:58:45, iannucci wrote:
> > > (that said, I dont see why we can't land this patch in upstream).
> > 
> > (( also, I'm looking at the rietveld code and I think I'm wrong... chromium
> > rietveld isn't currently blocking binary upload, though the CQ and TS still
> > appear not to support it)).
> 
> Rietveld is not blocking anything. To block the CQ from committing broken
files,
> depot_tools/rietveld.py naively tries to block anything that looks like binary
> because there has been issues in properly handling binary files. Issues looked
> like corrupted file and 0-length file. The code is in Rietveld.get_patch() and
> the exact same code is used by both apply_issue.py and the CQ.

Yeah, I'm just wrong. Sorry about that.

[Julie Parent, 2013-05-07 01:15:21]

Patch up for review with Rietveld: https://codereview.appspot.com/9183044/

On 2013/05/07 00:09:58, iannucci wrote:
> On 2013/05/07 00:06:23, Marc-Antoine Ruel wrote:
> > On 2013/05/07 00:02:01, iannucci wrote:
> > > On 2013/05/06 23:58:45, iannucci wrote:
> > > > (that said, I dont see why we can't land this patch in upstream).
> > > 
> > > (( also, I'm looking at the rietveld code and I think I'm wrong...
chromium
> > > rietveld isn't currently blocking binary upload, though the CQ and TS
still
> > > appear not to support it)).
> > 
> > Rietveld is not blocking anything. To block the CQ from committing broken
> files,
> > depot_tools/rietveld.py naively tries to block anything that looks like
binary
> > because there has been issues in properly handling binary files. Issues
looked
> > like corrupted file and 0-length file. The code is in Rietveld.get_patch()
and
> > the exact same code is used by both apply_issue.py and the CQ.
> 
> Yeah, I'm just wrong. Sorry about that.

[iannucci, 2013-05-07 23:53:52]

On 2013/05/07 01:15:21, Julie Parent wrote:
> Patch up for review with Rietveld: https://codereview.appspot.com/9183044/
> 
> On 2013/05/07 00:09:58, iannucci wrote:
> > On 2013/05/07 00:06:23, Marc-Antoine Ruel wrote:
> > > On 2013/05/07 00:02:01, iannucci wrote:
> > > > On 2013/05/06 23:58:45, iannucci wrote:
> > > > > (that said, I dont see why we can't land this patch in upstream).
> > > > 
> > > > (( also, I'm looking at the rietveld code and I think I'm wrong...
> chromium
> > > > rietveld isn't currently blocking binary upload, though the CQ and TS
> still
> > > > appear not to support it)).
> > > 
> > > Rietveld is not blocking anything. To block the CQ from committing broken
> > files,
> > > depot_tools/rietveld.py naively tries to block anything that looks like
> binary
> > > because there has been issues in properly handling binary files. Issues
> looked
> > > like corrupted file and 0-length file. The code is in Rietveld.get_patch()
> and
> > > the exact same code is used by both apply_issue.py and the CQ.
> > 
> > Yeah, I'm just wrong. Sorry about that.

This is landed upstream, so lgtm, feel free to commit.

[commit-bot: I haz the power, 2013-05-08 00:03:11]

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

[commit-bot: I haz the power, 2013-05-08 00:04:59]

Change committed as 198833

[cbiesinger, 2013-05-08 04:54:15]

https://codereview.chromium.org/14820015/diff/1/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/14820015/diff/1/third_party/upload.py#newcode...
third_party/upload.py:1389: is_binary = self.IsBinaryData(base_content) or
is_image
I'm sort of confused by this code, base_content is not initialized here in most
cases, right? It is initialized below, but it needs to know the value of
is_binary. How is this expected to work?

[iannucci, 2013-05-08 05:45:28]

On 2013/05/08 04:54:15, cbiesinger wrote:
> https://codereview.chromium.org/14820015/diff/1/third_party/upload.py
> File third_party/upload.py (right):
> 
>
https://codereview.chromium.org/14820015/diff/1/third_party/upload.py#newcode...
> third_party/upload.py:1389: is_binary = self.IsBinaryData(base_content) or
> is_image
> I'm sort of confused by this code, base_content is not initialized here in
most
> cases, right? It is initialized below, but it needs to know the value of
> is_binary. How is this expected to work?

Yeah this was mentioned in the other code review. Basically this whole function
is broken. You can't get base_content until you know if it's binary or not,
which, in turn, depends on the value of base_content.

That said, this will at least make it 'work' for images, which is what 99.99% of
reviewers care about for now.

[eseidel, 2013-06-08 00:17:35]

I'm told this made rietveld handle .svg files as binary, which breaks them being
able to land through the CQ?

[Julie Parent, 2013-06-08 00:18:35]

It doesn't effect the CQ, but I already have a patch for that up
for review: https://codereview.appspot.com/10117044/


On Fri, Jun 7, 2013 at 5:17 PM, <eseidel@chromium.org> wrote:

> I'm told this made rietveld handle .svg files as binary, which breaks them
> being
> able to land through the CQ?
>
>
https://codereview.chromium.**org/14820015/<https://codereview.chromium.org/1...
>