Remove "recursive" parameter from WebPageSerializer::serialize.

Remove "recursive" parameter from WebPageSerializer::serialize.

After a recent CL, almost all callers of
WebPageSerializer::serialize(WebLocalFrame*,...) pass recursive=false.  The
current CL removes the "recursive" parameter altogether and uses non-recursive
/ single-frame mode all the time.

The only place that kept using the recursive mode was 1 test in
dom_serializer_browsertest.cc.  This test has been moved to a new
SaveDownloadableIFrame test in save_page_browsertest.cc.  The new test tests
the same scenario as the old test, but coordinating serializing of multiple
frames at the SavePackage level (rather than at the WebPageSerializer level).

BUG=526786
Committed: https://crrev.com/d083d29530a507b7b5a6df1c572743c27c80c463
Cr-Commit-Position: refs/heads/master@{#351456}

[Łukasz Anforowicz, 2015-09-25 16:03:23]

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

[Łukasz Anforowicz, 2015-09-25 16:03:23]

Daniel, could you please take a look?

[dcheng, 2015-09-25 19:53:52]

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:335: found =
std::any_of(items.begin(), items.end(),
C++11 library features are still banned, aren't they?

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:340:
DownloadItemCreatedObserver(manager).WaitForDownloadItem(&items);
Hmm... maybe we should have a way to specify how many downloads we're expecting
/ what URLs? Otherwise, it seems like it could be non-deterministic how many
actual download items show up here, right?

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:783:
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SaveDownloadableIFrame) {
The original test wasn't marked as flaky, but this one is. What's the
difference?

https://codereview.chromium.org/1371533002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebPageSerializerImpl.cpp (right):

https://codereview.chromium.org/1371533002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebPageSerializerImpl.cpp:462: if (url.isValid()
|| m_localLinks.contains(url.string())) {
Should this be &&?

[Łukasz Anforowicz, 2015-09-25 20:35:57]

Daniel, can you take another look please?

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:335: found =
std::any_of(items.begin(), items.end(),
On 2015/09/25 19:53:52, dcheng wrote:
> C++11 library features are still banned, aren't they?

Thanks for catching this.  OTOH, based on the other piece of feedback, I will
revert VerifySavePackageExpectation changes.

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:340:
DownloadItemCreatedObserver(manager).WaitForDownloadItem(&items);
On 2015/09/25 19:53:52, dcheng wrote:
> Hmm... maybe we should have a way to specify how many downloads we're
expecting
> / what URLs? Otherwise, it seems like it could be non-deterministic how many
> actual download items show up here, right?

Based on the other piece of feedback, I will revert VerifySavePackageExpectation
changes
and dismiss the extra download item before kicking off save-page-as (see the
changes [vs previous patchset] in the body of the new test).

https://codereview.chromium.org/1371533002/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:783:
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SaveDownloadableIFrame) {
On 2015/09/25 19:53:52, dcheng wrote:
> The original test wasn't marked as flaky, but this one is. What's the
> difference?

This one interacts with the download manager (the other one was directly calling
WebPageSerializer).  I copied flakiness designation from another test in
save_page_browsertest.cc.

https://codereview.chromium.org/1371533002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebPageSerializerImpl.cpp (right):

https://codereview.chromium.org/1371533002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebPageSerializerImpl.cpp:462: if (url.isValid()
|| m_localLinks.contains(url.string())) {
On 2015/09/25 19:53:52, dcheng wrote:
> Should this be &&?

Right.  My bad.

[dcheng, 2015-09-25 21:28:21]

lgtm

https://codereview.chromium.org/1371533002/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1371533002/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:777: // Disabled on Windows due
to flakiness. http://crbug.com/162323
Let's try unconditionally enabling this test, so we'll find out if this new test
is flaky. And if it is, then we can fix it.

[Łukasz Anforowicz, 2015-09-25 21:38:28]

lukasza@chromium.org changed reviewers:
+ asanka@chromium.org

[Łukasz Anforowicz, 2015-09-25 21:38:28]

Asanka, could you please take a look?

https://codereview.chromium.org/1371533002/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1371533002/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:777: // Disabled on Windows due
to flakiness. http://crbug.com/162323
On 2015/09/25 21:28:21, dcheng wrote:
> Let's try unconditionally enabling this test, so we'll find out if this new
test
> is flaky. And if it is, then we can fix it.

Done.

[Łukasz Anforowicz, 2015-09-28 22:27:33]

lukasza@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Łukasz Anforowicz, 2015-09-28 22:27:33]

Randy, could you please take a look?

I know I have already sent out a CR request to Asanka, but this CL goes after a
CL you have reviewed earlier today

- crrev.com/1320453004 (Moving IPC messages from RV to RF;  this CL has already
been reviewed by you and is currently in CQ)

and before a CL that I'll be sending to you later

- crrev.com/1373573002 (Moving stitching of local paths from renderer to browser
process;  I'll be asking for a review soon [after getting sign-off from OOPIF
side)

so I think you might want to take a look.

[Randy Smith (Not in Mondays), 2015-09-29 15:02:18]

save_page_browsertest.cc, iframe-src-is-a-download.htm LGTM.  (I glanced at the
other browser side code, but not at the renderer side code.)

[Łukasz Anforowicz, 2015-09-29 16:31:23]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2015-09-29 16:31:24]

Charlie, could you please take a look?

For this CL, I mostly need sign-off for
    content/renderer/render_frame_impl.cc
    content/renderer/dom_serializer_browsertest.cc
        (the removed test is being moved to save_page_browsertest.cc)

This CL fits in-between ...:

- crrev.com/1320453004 (Moving IPC messages from RV to RF)

... and before a CL that I'll be sending to you later ...:

- crrev.com/1373573002 (Moving stitching of local paths from renderer to browser
process)

[Charlie Reis, 2015-09-29 18:23:44]

Great!  content/ LGTM.

[asanka, 2015-09-29 20:37:05]

Removing self since Randy's review should suffice.

[asanka, 2015-09-29 20:37:18]

asanka@chromium.org changed reviewers:
- asanka@chromium.org

[Łukasz Anforowicz, 2015-09-29 21:08:39]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-09-29 21:08:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1371533002/#ps60001
(title: "Rebasing...")

[commit-bot: I haz the power, 2015-09-29 21:10:12]

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

[commit-bot: I haz the power, 2015-09-29 21:20:38]

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

[commit-bot: I haz the power, 2015-09-29 21:20:39]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Łukasz Anforowicz, 2015-09-29 21:34:03]

On 2015/09/29 21:20:39, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

Hmmm... the failure linked above doesn't seem related to my changes:

    Connecting to https://codereview.chromium.org
    Downloading patch from https://codereview.chromium.org/1371533002/#ps60001
    Sending crash report ...
    ...
    urllib2.URLError: <urlopen error [Errno 10060] A connection attempt failed
because the connected party did not properly respond after a period of time, or
established connection failed because connected host has failed to respond>


Also - the patch applied cleanly and trybot passed here:
   
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...


So, I guess I'll just retry pushing the patch to CQ.

[Łukasz Anforowicz, 2015-09-29 21:34:08]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-09-29 21:53:44]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-09-29 21:59:16]

The CQ bit was unchecked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-09-29 22:59:09]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-09-29 23:00:21]

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

[commit-bot: I haz the power, 2015-09-30 00:43:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-30 00:44:27]

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