net: Remove UploadElement::TYPE_CHUNK

net: Remove UploadElement::TYPE_CHUNK

All chunk related information is handled by UploadData.
Now UploadElement has no idea about chunks.

BUG=None
TEST=net_unittests


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

[hashimoto, 2012-08-21 19:26:11]

willchan,
Could you review this?

[kinuko, 2012-08-22 01:47:10]

On 2012/08/21 19:26:11, hashimoto wrote:
> willchan,
> Could you review this?

afaik the mysterious chunk code in common_param_traits is added to fix
ChromeFrame net test, the one that is failing on win_rel.  Are we sure it's safe
to remove it?  http://codereview.chromium.org/6621024

(As for the cleanup I'm all for it)

[willchan no longer on Chromium, 2012-08-22 01:58:51]

Yeah, looks like this breaks ChromeFrame. Good thing the tests caught it.

[hashimoto, 2012-08-22 02:05:06]

On 2012/08/22 01:47:10, kinuko wrote:
> On 2012/08/21 19:26:11, hashimoto wrote:
> > willchan,
> > Could you review this?
> 
> afaik the mysterious chunk code in common_param_traits is added to fix
> ChromeFrame net test, the one that is failing on win_rel.  Are we sure it's
safe
> to remove it?  http://codereview.chromium.org/6621024
> 
> (As for the cleanup I'm all for it)

Thanks,
I think we should not have this code in common_param_traits, since the data gets
totally broken if we pass UploadData over IPC twice with this hack.
If we really want to support chunk upload in Chrome Frame, this code should be
implemented in UrlmonUploadDataStream instead and there should be another IPC to
send added chunks from browser process to Chrome Frame.
(I don't think it's worth doing, though.)

It seems we can safely disable the failing test, URLRequestTestHTTP
.TestPostChunkedDataBeforeStart, because we are already disabling
URLRequestTestHTTP .TestPostChunkedDataAfterStart
(see "Google Test filter" line of
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/54350/...)

[hashimoto, 2012-08-22 02:09:07]

On 2012/08/22 01:58:51, willchan wrote:
> Yeah, looks like this breaks ChromeFrame. Good thing the tests caught it.

Now I'm thinking to separate chunk upload tests from url_request_unittest.cc so
that they get not running with Chrome Frame tests, or adding a new #define for
chrome_frame.gyp to exclude the tests when it gets compiled for Chrome Frame
targets.

[willchan no longer on Chromium, 2012-08-22 02:10:52]

I don't feel comfortable straight up disabling the test. We should ask
someone who knows Chrome Frame.


On Tue, Aug 21, 2012 at 7:09 PM, <hashimoto@chromium.org> wrote:

> On 2012/08/22 01:58:51, willchan wrote:
>
>> Yeah, looks like this breaks ChromeFrame. Good thing the tests caught it.
>>
>
> Now I'm thinking to separate chunk upload tests from
> url_request_unittest.cc so
> that they get not running with Chrome Frame tests, or adding a new #define
> for
> chrome_frame.gyp to exclude the tests when it gets compiled for Chrome
> Frame
> targets.
>
>
http://codereview.chromium.**org/10861036/<http://codereview.chromium.org/108...
>

[hashimoto, 2012-08-22 02:23:48]

+ananta

Another possible solution is moving the hack to UrlmonUploadDataStream while NOT
implementing additional IPC to communicate newly added chunks.
In this way, we don't break TestPostChunkedDataBeforeStart (and we don't fix
TestPostChunkedDataAfterStart), nothing gets changed.

But, considering the purpose of chunk upload, I'm curious if the hack is worth
maintaining which enables chunk upload only when all chunks are appended before
the request starts.

[hashimoto, 2012-08-22 16:41:43]

New patch set with the hack revived.
No behavior change, no newly broken tests.

[satorux1, 2012-08-22 20:46:39]

The hack was moved from common_param_traits.cc to urlmon_url_request.cc, right?
I think the new place looks better to me.

BTW, I thought you could run chrome frame try bot. I forgot the name, though.

[hashimoto, 2012-08-22 20:57:07]

On 2012/08/22 20:46:39, satorux1 wrote:
> The hack was moved from common_param_traits.cc to urlmon_url_request.cc,
right?
Yes.
> I think the new place looks better to me.
> 
> BTW, I thought you could run chrome frame try bot. I forgot the name, though.
Other than win/win_rel? (they are already running chrome_frame_unittests and
chrome_frame_net_tests)

[ananta, 2012-08-22 21:05:09]

ChromeFrame changes LGTM. We could probably remove the upload support code from
chrome frame in a subsequent CL.

On 2012/08/22 20:57:07, hashimoto wrote:
> On 2012/08/22 20:46:39, satorux1 wrote:
> > The hack was moved from common_param_traits.cc to urlmon_url_request.cc,
> right?
> Yes.
> > I think the new place looks better to me.
> > 
> > BTW, I thought you could run chrome frame try bot. I forgot the name,
though.
> Other than win/win_rel? (they are already running chrome_frame_unittests and
> chrome_frame_net_tests)

[willchan no longer on Chromium, 2012-08-23 18:54:59]

LGTM

The last chunk stuff looks ugly to me, but you've basically just moved it, so
that seems fine. Please make sure ananta reviews the Chrome Frame stuff since I
don't know that part of the code.

[hashimoto, 2012-08-23 19:42:16]

avi,

Could you review the change for content/public/common/common_param_traits.cc?

[Avi (use Gerrit), 2012-08-23 20:43:45]

LGTM stamp

[commit-bot: I haz the power, 2012-08-23 21:33:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10861036/14003

[commit-bot: I haz the power, 2012-08-24 00:40:25]

Change committed as 153120