Merges the two different page serializers

This review merges the two existing page serializers, WebPageSerializerImpl and
PageSerializer, into one, PageSerializer. In addition to this it moves all
the old tests from WebPageNewSerializerTest and WebPageSerializerTest to the
PageSerializerTest structure and splits out one test for MHTML into a new
MHTMLTest file.

Saving as 'Webpage, Complete', 'Webpage, HTML Only' and as MHTML when the
'Save Page as MHTML' flag is enabled now uses the same code, and should thus
have the same feature set. Meaning that both modes now should be a bit better.

Detailed list of changes:

- PageSerializerTest: Prepare for more DTD test
- PageSerializerTest: Remove now unneccesary input image test
- PageSerializerTest: Remove unused WebPageSerializer/Impl code
- PageSerializerTest: Move data URI morph test
- PageSerializerTest: Move data URI test
- PageSerializerTest: Move namespace test
- PageSerializerTest: Move SVG Image test
- MHTMLTest: Move MHTML specific test to own test file
- PageSerializerTest: Delete duplicate XML header test
- PageSerializerTest: Move blank frame test
- PageSerializerTest: Move CSS test
- PageSerializerTest: Add frameset/frame test
- PageSerializerTest: Move old iframe test
- PageSerializerTest: Move old elements test
- Use PageSerizer for saving web pages
- PageSerializerTest: Test for rewriting links
- PageSerializer: Add rewrite link accumulator
- PageSerializer: Serialize images in iframes/frames src
- PageSerializer: XHTML fix for meta tags
- PageSerializer: Add presentation CSS
- PageSerializer: Rename out parameter

BUG=328354
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162155

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162294

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164094

[abarth-chromium, 2013-11-14 16:55:02]

This looks great!!!  Thank you for paying down this debt.  LGTM.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:65: #include "wtf/OwnPtr.h"
Undoubtedly one of these other headers pulls in OwnPtr.h :)

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:124: 
This blank line is not needed.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:181: 
You've got two extra blank lines here.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:206: // TODO: or move to append open tag /
close tag
TODO -> FIXME

Also, please use complete sentences in comments.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:243: // Get the absolute link
You can remove these comments that just way what the code does.  We can
determine that by looking at the code.  Comments that explain why the code does
what it does are useful, however.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:248: // TODO: refactor markupaccumulator,
reuse code
TODO -> FIXME

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:265: 
You've got an extra blank line here.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:304:
WTF::OwnPtr<SerializerMarkupAccumulator> accumulator;
No need for WTF:: here.  OwnPtr has a using WTF::OwnPtr directive in the header.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:309: }
No need for { } in either branch of the if here.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.cpp:489: 
No need for this blank line.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
File Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.h:60: typedef HashMap<WTF::String, WTF::String>
LinkLocalPathMap;
WTF::String -> String.

There's no need to use the WTF namespace for String.  There's a using
WTF::String directive in the header.

https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.h:66: explicit
PageSerializer(Vector<SerializedResource>*, HashMap<WTF::String, WTF::String>* =
0, WTF::String = "");
Why not use the LinkLocalPathMap type here?

No need for the explicit keyword anymore.  That only does something for
one-argument constructors.

https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer.cpp
File Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer....
Source/web/WebPageSerializer.cpp:114: // Build local resources map.
None of these comments are worth having.

https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer....
Source/web/WebPageSerializer.cpp:117: HashMap<WTF::String, WTF::String>
m_localLinks;
Why not use the typedef you created for this purpose?

https://codereview.chromium.org/68613003/diff/1/public/web/WebPageSerializerC...
File public/web/WebPageSerializerClient.h (right):

https://codereview.chromium.org/68613003/diff/1/public/web/WebPageSerializerC...
public/web/WebPageSerializerClient.h:65: const WebCString& data,
PageSerializationStatus) = 0;
What's the benefit of reformatting this line?  The old one was easier for me to
read.

[Tiger, 2013-11-15 11:07:27]

On 2013/11/14 16:55:02, abarth wrote:
> This looks great!!!  Thank you for paying down this debt.  LGTM.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> File Source/core/page/PageSerializer.cpp (right):
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:65: #include "wtf/OwnPtr.h"
> Undoubtedly one of these other headers pulls in OwnPtr.h :)
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:124: 
> This blank line is not needed.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:181: 
> You've got two extra blank lines here.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:206: // TODO: or move to append open tag /
> close tag
> TODO -> FIXME
> 
> Also, please use complete sentences in comments.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:243: // Get the absolute link
> You can remove these comments that just way what the code does.  We can
> determine that by looking at the code.  Comments that explain why the code
does
> what it does are useful, however.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:248: // TODO: refactor markupaccumulator,
> reuse code
> TODO -> FIXME
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:265: 
> You've got an extra blank line here.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:304:
> WTF::OwnPtr<SerializerMarkupAccumulator> accumulator;
> No need for WTF:: here.  OwnPtr has a using WTF::OwnPtr directive in the
header.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:309: }
> No need for { } in either branch of the if here.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.cpp:489: 
> No need for this blank line.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> File Source/core/page/PageSerializer.h (right):
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.h:60: typedef HashMap<WTF::String,
WTF::String>
> LinkLocalPathMap;
> WTF::String -> String.
> 
> There's no need to use the WTF namespace for String.  There's a using
> WTF::String directive in the header.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.h:66: explicit
> PageSerializer(Vector<SerializedResource>*, HashMap<WTF::String, WTF::String>*
=
> 0, WTF::String = "");
> Why not use the LinkLocalPathMap type here?
> 
> No need for the explicit keyword anymore.  That only does something for
> one-argument constructors.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer.cpp
> File Source/web/WebPageSerializer.cpp (right):
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer....
> Source/web/WebPageSerializer.cpp:114: // Build local resources map.
> None of these comments are worth having.
> 
>
https://codereview.chromium.org/68613003/diff/1/Source/web/WebPageSerializer....
> Source/web/WebPageSerializer.cpp:117: HashMap<WTF::String, WTF::String>
> m_localLinks;
> Why not use the typedef you created for this purpose?
> 
>
https://codereview.chromium.org/68613003/diff/1/public/web/WebPageSerializerC...
> File public/web/WebPageSerializerClient.h (right):
> 
>
https://codereview.chromium.org/68613003/diff/1/public/web/WebPageSerializerC...
> public/web/WebPageSerializerClient.h:65: const WebCString& data,
> PageSerializationStatus) = 0;
> What's the benefit of reformatting this line?  The old one was easier for me
to
> read.

Thanks! I've fixed the issues here and one or two I noticed as well. 

In addition to this I changed the signature of the serialize method in
WebPageSerializer.h to the original version and made a small adjustment in the
corresponding .cpp file, thus eliminating the need for changes in Chromium this
time.

One could probably spend a bit more time refactoring this and also move some of
the links collecting and local paths from Chromium to Blink, and simplify the
APIs a bit.

[abarth-chromium, 2013-11-15 15:53:56]

On 2013/11/15 11:07:27, Tiger wrote:
> One could probably spend a bit more time refactoring this and also move some
of
> the links collecting and local paths from Chromium to Blink, and simplify the
> APIs a bit.

Those sound like valuable things to do.  I'd recommend doing them in another CL.
 This CL is already quite big and would be good get into the tree.

[abarth-chromium, 2013-11-15 15:58:50]

LGTM

I'm going to put this in the CQ so that it lands, but would you be willing to
write a cleanup CL that fixes the (minor!) issues below?  Thanks!

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:188: // local_links_ include all pair of
local resource path and corresponding
What is local_links_?  Do you mean m_replaceLinks?

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:238: &&
!element->isJavaScriptURLAttribute(attribute)) {
You can merge these lines.  There's no 80 col limit in Blink.

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:249: result.append(m_directoryName);
Do we need to escape the directory name at all?

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:260: 
no need for this blank line

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:267: 
ditto

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
File Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/68613003/diff/120001/Source/core/page/PageSer...
Source/core/page/PageSerializer.h:66:
PageSerializer(Vector<SerializedResource>*, LinkLocalPathMap* = 0, String = "");
Can you name this last parameter?  It's not clear from context what it's for.

[commit-bot: I haz the power, 2013-11-15 15:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/120001

[commit-bot: I haz the power, 2013-11-15 16:48:21]

Retried try job too often on linux_blink for step(s) blink_platform_unittests,
webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-15 23:46:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/360001

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/360001

[commit-bot: I haz the power, 2013-11-16 01:37:31]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-16 13:09:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/630001

[commit-bot: I haz the power, 2013-11-16 15:15:04]

Change committed as 162155

[eae, 2013-11-16 19:19:03]

This change broke compilation on all platforms, reverted in r162156.

[abarth-chromium, 2013-11-16 20:37:40]

On 2013/11/16 19:19:03, eae wrote:
> This change broke compilation on all platforms, reverted in r162156.

Was there some sort of commit race?  The try bot bubbles are all green...

[eae, 2013-11-17 00:36:17]

On 2013/11/16 20:37:40, abarth wrote:
> On 2013/11/16 19:19:03, eae wrote:
> > This change broke compilation on all platforms, reverted in r162156.
> 
> Was there some sort of commit race?  The try bot bubbles are all green...

The commit queue latency is the real culprit here, all bots passed many hours
before the CQ finally got around to land it.

[Tiger, 2013-11-19 11:24:13]

On 2013/11/17 00:36:17, eae wrote:
> On 2013/11/16 20:37:40, abarth wrote:
> > On 2013/11/16 19:19:03, eae wrote:
> > > This change broke compilation on all platforms, reverted in r162156.
> > 
> > Was there some sort of commit race?  The try bot bubbles are all green...
> 
> The commit queue latency is the real culprit here, all bots passed many hours
> before the CQ finally got around to land it.

The problem was that there are unit test for the page serializer in the chrome
as well (content/renderer/dom_serializer_browsertests.cc). I just built chrome,
which doesn't include those unit tests. The unit tests seems to be crashing at
the moment even without this patch so I'll go ahead and push this with a compile
fix.

Most of those unit tests should really be moved to blink.

[Tiger, 2013-11-19 11:25:03]

On 2013/11/19 11:24:13, Tiger wrote:
> On 2013/11/17 00:36:17, eae wrote:
> > On 2013/11/16 20:37:40, abarth wrote:
> > > On 2013/11/16 19:19:03, eae wrote:
> > > > This change broke compilation on all platforms, reverted in r162156.
> > > 
> > > Was there some sort of commit race?  The try bot bubbles are all green...
> > 
> > The commit queue latency is the real culprit here, all bots passed many
hours
> > before the CQ finally got around to land it.
> 
> The problem was that there are unit test for the page serializer in the chrome
> as well (content/renderer/dom_serializer_browsertests.cc). I just built
chrome,
> which doesn't include those unit tests. The unit tests seems to be crashing at
> the moment even without this patch so I'll go ahead and push this with a
compile
> fix.
> 
> Most of those unit tests should really be moved to blink.

And the try bots doesn't run or build those tests for blink either, so. That's
why it was green, but didn't compile at a later stage.

[commit-bot: I haz the power, 2013-11-19 11:25:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/770001

[commit-bot: I haz the power, 2013-11-19 12:46:15]

Change committed as 162294

[abarth-chromium, 2013-11-19 15:52:30]

On 2013/11/19 11:24:13, Tiger wrote:
> The problem was that there are unit test for the page serializer in the chrome
> as well (content/renderer/dom_serializer_browsertests.cc). I just built
chrome,
> which doesn't include those unit tests. The unit tests seems to be crashing at
> the moment even without this patch so I'll go ahead and push this with a
compile
> fix.
> 
> Most of those unit tests should really be moved to blink.

If we're not running them on the bots, we should probably just delete them. 
They're providing negative value in their current state.

[jamesr, 2013-11-21 20:44:18]

On 2013/11/19 15:52:30, abarth wrote:
> On 2013/11/19 11:24:13, Tiger wrote:
> > The problem was that there are unit test for the page serializer in the
chrome
> > as well (content/renderer/dom_serializer_browsertests.cc). I just built
> chrome,
> > which doesn't include those unit tests. The unit tests seems to be crashing
at
> > the moment even without this patch so I'll go ahead and push this with a
> compile
> > fix.
> > 
> > Most of those unit tests should really be moved to blink.
> 
> If we're not running them on the bots, we should probably just delete them. 
> They're providing negative value in their current state.

Why do you say that they aren't running on the bots?
dom_serializer_browsertest.cc is linked into content_browsertests which is most
certainly run on many bots.

[abarth-chromium, 2013-11-21 20:50:05]

On 2013/11/21 20:44:18, jamesr wrote:
> On 2013/11/19 15:52:30, abarth wrote:
> > If we're not running them on the bots, we should probably just delete them. 
> > They're providing negative value in their current state.
> 
> Why do you say that they aren't running on the bots?
> dom_serializer_browsertest.cc is linked into content_browsertests which is
most
> certainly run on many bots.

Tiger said above that they were already crashing before this CL.  Maybe he's
mistaken?

[Tiger, 2013-11-21 22:09:24]

On 2013/11/21 20:44:18, jamesr wrote:
> On 2013/11/19 15:52:30, abarth wrote:
> > On 2013/11/19 11:24:13, Tiger wrote:
> > > The problem was that there are unit test for the page serializer in the
> chrome
> > > as well (content/renderer/dom_serializer_browsertests.cc). I just built
> > chrome,
> > > which doesn't include those unit tests. The unit tests seems to be
crashing
> at
> > > the moment even without this patch so I'll go ahead and push this with a
> > compile
> > > fix.
> > > 
> > > Most of those unit tests should really be moved to blink.
> > 
> > If we're not running them on the bots, we should probably just delete them. 
> > They're providing negative value in their current state.
> 
> Why do you say that they aren't running on the bots?
> dom_serializer_browsertest.cc is linked into content_browsertests which is
most
> certainly run on many bots.

Yes, you are right. What I meant to say was that they aren't run by default for
Blink CLs (like this one). It didn't cross my mind to run the
content_browsertest locally when doing this changes, and this completely missed
that there where some unit tests covering page serialization in the content
browser test in chromium.

As for them crashing -- I wrongly assumed that since those specific test where
crashing on my machine both before and after my changes that was the case on the
trybots as well, and that the test was unused and outdated. But the trybots
manage to compile and run those test just fine, and they are not marked as
crashing so I should have known this I guess. Locally for me running this
crashes in cairo_surface_get_font_options, and I haven't managed to figure out
why yet. Any help there would be appreciated.

Full call stack here: http://pastebin.com/aEWnpkT5

[Tiger, 2013-12-13 12:19:55]

I got this up and running in a VM, test are flaky here (Ubuntu 32-bit, 12.04.1),
but at least works some of the time so I could solve this.

Disabling some tests in a review <https://codereview.chromium.org/115163002>
here, will enable with fixes once this is in. It would be great if someone could
take a look at that review, and a second look at this again.

[Tiger, 2013-12-17 09:24:33]

On 2013/12/13 12:19:55, Tiger wrote:
> I got this up and running in a VM, test are flaky here (Ubuntu 32-bit,
12.04.1),
> but at least works some of the time so I could solve this.
> 
> Disabling some tests in a review <https://codereview.chromium.org/115163002>
> here, will enable with fixes once this is in. It would be great if someone
could
> take a look at that review, and a second look at this again.

Since I only did some minor changes I'll try to land this today.

[commit-bot: I haz the power, 2013-12-17 09:25:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/1150001

[commit-bot: I haz the power, 2013-12-17 09:25:41]

Failed to apply patch for Source/core/page/PageSerializer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/PageSerializer.cpp
  Hunk #1 FAILED at 53.
  1 out of 8 hunks FAILED -- saving rejects to file
Source/core/page/PageSerializer.cpp.rej

Patch:       Source/core/page/PageSerializer.cpp
Index: Source/core/page/PageSerializer.cpp
diff --git a/Source/core/page/PageSerializer.cpp
b/Source/core/page/PageSerializer.cpp
index
d41f871976383f86d0b3e5e170d4357621bda884..c63bd915a2132907c3cd4afbd092feef5a58877a
100644
--- a/Source/core/page/PageSerializer.cpp
+++ b/Source/core/page/PageSerializer.cpp
@@ -53,6 +53,7 @@
 #include "core/html/HTMLInputElement.h"
 #include "core/html/HTMLLinkElement.h"
 #include "core/html/HTMLStyleElement.h"
+#include "core/html/ImageDocument.h"
 #include "core/html/parser/HTMLMetaCharsetParser.h"
 #include "core/frame/Frame.h"
 #include "core/page/Page.h"
@@ -105,12 +106,11 @@ public:
     virtual ~SerializerMarkupAccumulator();
 
 protected:
-    virtual void appendText(StringBuilder& out, Text*);
-    virtual void appendElement(StringBuilder& out, Element*, Namespaces*);
-    virtual void appendCustomAttributes(StringBuilder& out, Element*,
Namespaces*);
-    virtual void appendEndTag(Node*);
+    virtual void appendText(StringBuilder&, Text*) OVERRIDE;
+    virtual void appendElement(StringBuilder&, Element*, Namespaces*) OVERRIDE;
+    virtual void appendCustomAttributes(StringBuilder&, Element*, Namespaces*)
OVERRIDE;
+    virtual void appendEndTag(Node*) OVERRIDE;
 
-private:
     PageSerializer* m_serializer;
     Document* m_document;
 };
@@ -126,28 +126,34 @@
SerializerMarkupAccumulator::~SerializerMarkupAccumulator()
 {
 }
 
-void SerializerMarkupAccumulator::appendText(StringBuilder& out, Text* text)
+void SerializerMarkupAccumulator::appendText(StringBuilder& result, Text* text)
 {
     Element* parent = text->parentElement();
     if (parent && !shouldIgnoreElement(parent))
-        MarkupAccumulator::appendText(out, text);
+        MarkupAccumulator::appendText(result, text);
 }
 
-void SerializerMarkupAccumulator::appendElement(StringBuilder& out, Element*
element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendElement(StringBuilder& result, Element*
element, Namespaces* namespaces)
 {
     if (!shouldIgnoreElement(element))
-        MarkupAccumulator::appendElement(out, element, namespaces);
+        MarkupAccumulator::appendElement(result, element, namespaces);
 
+    // FIXME: Refactor MarkupAccumulator so it is easier to append an element
like this, without special cases for XHTML
     if (element->hasTagName(HTMLNames::headTag)) {
-        out.append("<meta charset=\"");
-        out.append(m_document->charset());
-        out.append("\">");
+        result.appendLiteral("<meta http-equiv=\"Content-Type\" content=\"");
+        result.append(m_document->suggestedMIMEType());
+        result.appendLiteral("; charset=");
+        result.append(m_document->charset());
+        if (m_document->isXHTMLDocument())
+            result.appendLiteral("\" />");
+        else
+            result.appendLiteral("\">");
     }
 
     // FIXME: For object (plugins) tags and video tag we could replace them by
an image of their current contents.
 }
 
-void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& out,
Element* element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& result,
Element* element, Namespaces* namespaces)
 {
     if (!element->isFrameOwnerElement())
         return;
@@ -163,7 +169,7 @@ void
SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& out, Ele
 
     // We need to give a fake location to blank frames so they can be
referenced by the serialized frame.
     url = m_serializer->urlForBlankFrame(frame);
-    appendAttribute(out, element,
Attribute(frameOwnerURLAttributeName(*frameOwner), url.string()), namespaces);
+    appendAttribute(result, element,
Attribute(frameOwnerURLAttributeName(*frameOwner), url.string()), namespaces);
 }
 
 void SerializerMarkupAccumulator::appendEndTag(Node* node)
@@ -172,8 +178,89 @@ void SerializerMarkupAccumulator::appendEndTag(Node* node)
         MarkupAccumulator::appendEndTag(node);
 }
 
-PageSerializer::PageSerializer(Vector<SerializedResource>* resources)
+class LinkChangeSerializerMarkupAccumulator : public
SerializerMarkupAccumulator {
+public:
+    LinkChangeSerializerMarkupAccumulator(PageSerializer*, Document*,
Vector<Node*>*, LinkLocalPathMap*, String);
+
+protected:
+    virtual void appendElement(StringBuilder&, Element*, Namespaces*) OVERRIDE;
+    virtual void appendAttribute(StringBuilder&, Element*, const Attribute&,
Namespaces*) OVERRIDE;
+
+private:
+    // m_replaceLinks include all pair of local resource path and corresponding
original link.
+    LinkLocalPathMap* m_replaceLinks;
+    String m_directoryName;
+};
+
+LinkChangeSerializerMarkupAccumulator::LinkChangeSerializerMarkupAccumulator(PageSerializer*
serializer, Document* document, Vector<Node*>* nodes, LinkLocalPathMap* links,
String directoryName)
+    : SerializerMarkupAccumulator(serializer, document, nodes)
+    , m_replaceLinks(links)
+    , m_directoryName(directoryName)
+{
+}
+
+void LinkChangeSerializerMarkupAccumulator::appendElement(StringBuilder&
result, Element* element, Namespaces* namespaces)
+{
+    // FIXME: We could move the uncommenting to appendOpenTag and
appendCloseTag, or just remove it
+    if (element->hasTagName(HTMLNames::baseTag)) {
+        // Comment the BASE tag when serializing dom.
+        result.append("<!--");
+    } else if (element->hasTagName(HTMLNames::htmlTag)) {
+        // Add MOTW (Mark of the Web) declaration before html tag.
+        // See http://msdn2.microsoft.com/en-us/library/ms537628(VS.85).aspx.
+        result.append(String::format("\n<!-- saved from url=(%04d)%s -->\n",
+            static_cast<int>(m_document->url().string().utf8().length()),
+            m_document->url().string().utf8().data()));
+    }
+
+    SerializerMarkupAccumulator::appendElement(result, element, namespaces);
+
+    if (element->hasTagName(HTMLNames::baseTag)) {
+        // Comment the BASE tag when serializing dom.
+        result.appendLiteral("-->");
+
+        // FIXME: Refactor MarkupAccumulator so it is easier to append an
element like this, without special cases for XHTML
+        // Append a new base tag declaration.
+        result.appendLiteral("<base href=\".\"");
+        if (!m_document->baseTarget().isEmpty()) {
+            result.appendLiteral(" target=\"");
+            result.append(m_document->baseTarget());
+            result.append('"');
+        }
+        if (m_document->isXHTMLDocument())
+            result.appendLiteral(" />");
+        else
+            result.appendLiteral(">");
+    }
+}
+void LinkChangeSerializerMarkupAccumulator::appendAttribute(StringBuilder&
result, Element* element, const Attribute& attribute, Namespaces* namespaces)
+{
+    if (m_replaceLinks && element->isURLAttribute(attribute) &&
!element->isJavaScriptURLAttribute(attribute)) {
+
+        String completeURL = m_document->completeURL(attribute.value());
+
+        if (m_replaceLinks->contains(completeURL)) {
+            // FIXME: Refactor MarkupAccumulator so it is easier to append an
attribute like this.
+            result.append(' ');
+            result.append(attribute.name().toString());
+            result.appendLiteral("=\"");
+            if (!m_directoryName.isEmpty()) {
+                result.appendLiteral("./");
+                result.append(m_directoryName);
+                result.append('/');
+            }
+            result.append(m_replaceLinks->get(completeURL));
+            result.appendLiteral("\"");
+            return;
+        }
+    }
+    MarkupAccumulator::appendAttribute(result, element, attribute, namespaces);
+}
+
+PageSerializer::PageSerializer(Vector<SerializedResource>* resources,
LinkLocalPathMap* urls, String directory)
     : m_resources(resources)
+    , m_URLs(urls)
+    , m_directory(directory)
     , m_blankFrameCounter(0)
 {
 }
@@ -199,15 +286,21 @@ void PageSerializer::serializeFrame(Frame* frame)
         return;
     }
 
-    Vector<Node*> nodes;
-    SerializerMarkupAccumulator accumulator(this, document, &nodes);
-    WTF::TextEncoding textEncoding(document->charset());
-    CString data;
-    if (!textEncoding.isValid()) {
-        // FIXME: iframes used as images trigger this. We should deal with them
correctly.
+    // If frame is an image document, add the image and don't continue
+    if (document->isImageDocument()) {
+        ImageDocument* imageDocument = toImageDocument(document);
+        addImageToResources(imageDocument->cachedImage(),
imageDocument->imageElement()->renderer(), url);
         return;
     }
-    String text = accumulator.serializeNodes(document, IncludeNode);
+
+    Vector<Node*> nodes;
+    OwnPtr<SerializerMarkupAccumulator> accumulator;
+    if (m_URLs)
+        accumulator = adoptPtr(new LinkChangeSerializerMarkupAccumulator(this,
document, &nodes, m_URLs, m_directory));
+    else
+        accumulator = adoptPtr(new SerializerMarkupAccumulator(this, document,
&nodes));
+    String text = accumulator->serializeNodes(document, IncludeNode);
+    WTF::TextEncoding textEncoding(document->charset());
     CString frameHTML = textEncoding.normalizeAndEncode(text,
WTF::EntitiesForUnencodables);
     m_resources->append(SerializedResource(url, document->suggestedMIMEType(),
SharedBuffer::create(frameHTML.data(), frameHTML.length())));
     m_resourceURLs.add(url);
@@ -219,8 +312,10 @@ void PageSerializer::serializeFrame(Frame* frame)
 
         Element* element = toElement(node);
         // We have to process in-line style as it might contain some resources
(typically background images).
-        if (element->isStyledElement())
+        if (element->isStyledElement()…
(message too large)

[commit-bot: I haz the power, 2013-12-17 12:24:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/1170001

[commit-bot: I haz the power, 2013-12-17 13:58:22]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Tiger, 2013-12-17 21:04:07]

Waiting for more Chromium fixes in: https://codereview.chromium.org/106023004/

[commit-bot: I haz the power, 2013-12-18 13:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/68613003/1180001

[commit-bot: I haz the power, 2013-12-18 15:36:36]

Change committed as 164094

[abarth-chromium, 2013-12-19 07:43:14]

sgtm