[webkit-reviews] review denied: [Bug 58947] [Chromium] Add a way to serialize a WebView back to HTML : [Attachment 90562] Moved code to WebCore.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jay Civelli <jcivelli at chromium.org>'s
request for review:
Bug 58947: [Chromium] Add a way to serialize a WebView back to HTML
https://bugs.webkit.org/show_bug.cgi?id=58947

Attachment 90562: Moved code to WebCore.
https://bugs.webkit.org/attachment.cgi?id=90562&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90562&action=review

This looks great.  A few minor comments.

> Source/WebCore/page/PageSerializer.cpp:56
> +namespace {

Apparently we're supposed to use the static keyword in favor of anonymous
namespaces, but I see that you're using this because you want the
SerializerMarkupAccumulator class to have internal linkage.

> Source/WebCore/page/PageSerializer.cpp:127
> +    url = m_serializer->urlForBlankFrame(frame);
> +    RefPtr<Attribute> attribute = Attribute::create(HTMLNames::srcAttr,
url.string());
> +    appendAttribute(out, element, *attribute, namespaces);

This isn't quite correct in the case of an <object> tag, which uses the data
attribute for the frame's URL.	HTMLFrameElementBase::isURLAttribute exists to
solve this problem in other places.  See also
HTMLObjectElement::imageSourceAttributeName for a slightly different solution
to a similar problem.

> Source/WebCore/page/PageSerializer.cpp:146
> +

extra blank line here

> Source/WebCore/page/PageSerializer.cpp:163
> +    if (!url.isValid() || url == blankURL()) {

This should use the protocolIs check instead of comparing against blankURL.

> Source/WebCore/page/PageSerializer.cpp:168
> +    if (m_resourceURLs.contains(url)) {

Maybe we should convert all the URLs be unique?

> Source/WebCore/page/PageSerializer.cpp:191
> +	       HTMLImageElement* imageElem =
static_cast<HTMLImageElement*>(element);

imageElem => imageElement (we don't like abbreviations)

> Source/WebCore/page/PageSerializer.cpp:193
> +	       CachedImage* cachedImg = imageElem->cachedImage();

cachedImage.

> Source/WebCore/page/PageSerializer.cpp:199
> +		   KURL url =
document->completeURL(linkElement->getAttribute(HTMLNames::hrefAttr));

This isn't really correct.  There might have been a redirect.  You probably
want StyleSheet::finalURL, but that might cause problems when loading the style
sheet...  I guess it depends on how we deserialize these.  I'm not sure what to
do here.

> Source/WebCore/page/PageSerializer.cpp:207
> +		   serializeCSSStyleSheet(static_cast<CSSStyleSheet*>(sheet),
KURL());

What about style attributes?  Seems like you'll want to grab the images and
such out of those as well.

> Source/WebCore/page/PageSerializer.cpp:217
> +    String cssText;

We should use StringBuilder instead.  Calling append a lot on String is slow.

> Source/WebCore/page/PageSerializer.cpp:307
> +    String url("wyciwyg://frame/");
> +    url.append(String::number(m_blankFrameCounter++));
> +    KURL fakeURL(ParsedURLString, url);

You should use makeString instead of calling append.  I'm not sure it matters,
but it's the new hotness.

> Source/WebCore/page/PageSerializer.h:92
> +    // The list of resources retreived so far.
> +    Vector<Resource>* m_resources;
> +
> +    // The list of URLs we have retrieved so far, use to ensure we only
retrieved each resource once.
> +    ListHashSet<KURL> m_resourceURLs;
> +
> +    // The URLs used to identify blank frames.
> +    HashMap<Frame*, KURL> m_blankFrameURLs;
> +
> +    // Used to generate unique fake URLs for blank frames.
> +    unsigned m_blankFrameCounter;

We usually skip these comments and remove all the blank lines between member
variables.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:195
> +	   resource.data = WebCString(iter->data->data(), iter->data->size());

This creates a copy of all the images?	There isn't a way to just take a
reference to the SharedData ?