Fix SavePageAsMHTMLBrowserTest

Fix SavePageAsMHTMLBrowserTest
BUG=127527
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137248

[benjhayden, 2012-05-14 15:19:40]

PTAL

[Randy Smith (Not in Mondays), 2012-05-14 17:55:19]

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:158: return ((info.url == url_)
&&
My personal feeling is that parens around the == is overkill, but it's up to
you.

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:161: // number of files.
Instead of just putting in an off switch, can you replumb this path so that
info.received_bytes is called something like "progress_count" and then pass in
either the expected number of files or the expected number of bytes depending on
the test?

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:414: EXPECT_LE(2758,
actual_file_size);
I dislike this type of naked constant.  Is there some way to derive this value
(probe the file system for the files we're actually saving)?  If not, I do think
we should keep it as a defined constant.

[benjhayden, 2012-05-14 18:51:37]

kFileSizeMin

[benjhayden, 2012-05-14 19:27:19]

parens

[benjhayden, 2012-05-14 19:27:37]

PTAL

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:158: return ((info.url == url_)
&&
On 2012/05/14 17:55:19, rdsmith wrote:
> My personal feeling is that parens around the == is overkill, but it's up to
> you.

My personal feeling is that ungrouped conditions are living dangerously, but
Done.

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:161: // number of files.
On 2012/05/14 17:55:19, rdsmith wrote:
> Instead of just putting in an off switch, can you replumb this path so that
> info.received_bytes is called something like "progress_count" and then pass in
> either the expected number of files or the expected number of bytes depending
on
> the test?  

Renaming the variable won't fix the problem.
The problem is that SavePageAsMHTML doesn't know the expected number of bytes.
It could be 2759 or it could be 2760. It probably won't be a different number
until the year 10,000, but I'm not sure.
I could make it calculate the expected number of bytes based on the date, but
that seems a little silly. Or perhaps it just highlights the existing silliness.
I'd like to be able to set a flag to disable the date header in the mhtml
generator (I did something like this in video), but that's probably an
"abstraction violation".
I'd like to parse the generated mhtml and expect some of the headers and a hash
of the body, but that seems too heavy. I did something like this in video
("video_digest"), but it was many-purposed, whereas such a parser here would be
for this one test.

http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:414: EXPECT_LE(2758,
actual_file_size);
On 2012/05/14 17:55:19, rdsmith wrote:
> I dislike this type of naked constant.  Is there some way to derive this value
> (probe the file system for the files we're actually saving)?  If not, I do
think
> we should keep it as a defined constant.

Done.

[Randy Smith (Not in Mondays), 2012-05-14 20:09:56]

Gotcha.  This all makes sense.  LGTM.

On 2012/05/14 19:27:37, benjhayden_chromium wrote:
> PTAL
> 
>
http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
> File chrome/browser/download/save_page_browsertest.cc (right):
> 
>
http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
> chrome/browser/download/save_page_browsertest.cc:158: return ((info.url ==
url_)
> &&
> On 2012/05/14 17:55:19, rdsmith wrote:
> > My personal feeling is that parens around the == is overkill, but it's up to
> > you.
> 
> My personal feeling is that ungrouped conditions are living dangerously, but
> Done.
> 
>
http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
> chrome/browser/download/save_page_browsertest.cc:161: // number of files.
> On 2012/05/14 17:55:19, rdsmith wrote:
> > Instead of just putting in an off switch, can you replumb this path so that
> > info.received_bytes is called something like "progress_count" and then pass
in
> > either the expected number of files or the expected number of bytes
depending
> on
> > the test?  
> 
> Renaming the variable won't fix the problem.
> The problem is that SavePageAsMHTML doesn't know the expected number of bytes.
> It could be 2759 or it could be 2760. It probably won't be a different number
> until the year 10,000, but I'm not sure.
> I could make it calculate the expected number of bytes based on the date, but
> that seems a little silly. Or perhaps it just highlights the existing
silliness.
> I'd like to be able to set a flag to disable the date header in the mhtml
> generator (I did something like this in video), but that's probably an
> "abstraction violation".
> I'd like to parse the generated mhtml and expect some of the headers and a
hash
> of the body, but that seems too heavy. I did something like this in video
> ("video_digest"), but it was many-purposed, whereas such a parser here would
be
> for this one test.
> 
>
http://codereview.chromium.org/10392082/diff/1/chrome/browser/download/save_p...
> chrome/browser/download/save_page_browsertest.cc:414: EXPECT_LE(2758,
> actual_file_size);
> On 2012/05/14 17:55:19, rdsmith wrote:
> > I dislike this type of naked constant.  Is there some way to derive this
value
> > (probe the file system for the files we're actually saving)?  If not, I do
> think
> > we should keep it as a defined constant.
> 
> Done.

[commit-bot: I haz the power, 2012-05-15 19:17:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10392082/7002

[commit-bot: I haz the power, 2012-05-15 21:20:56]

Change committed as 137248

[Hironori Bono, 2012-05-16 03:41:57]

LGTM2.
Thanks for your fix.

Regards,

Hironori Bono