[Webkit-unassigned] [Bug 14959] No back forward entry added for pages created in javascript

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=14959





--- Comment #30 from Adam Barth <abarth at webkit.org>  2012-03-27 16:33:19 PST ---
(From update of attachment 134004)
View in context: https://bugs.webkit.org/attachment.cgi?id=134004&action=review

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:128
> +    // See bug <https://bugs.webkit.org/show_bug.cgi?id=14959> for discussion.

We don't usually put comments like this in the source files.  We use "svn annotate" and ChangeLogs to find the bug threads about the code.

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:129
> +    UString firstString = exec->argument(0).toString(exec)->value(exec);

What if toString throws an exception?

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:132
> +    if (mimeType != "text/plain")

This check doesn't seem like it can be right.  What if the mimeType is "text/plain; charset=utf-8" ?

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:133
> +            mimeType = "text/html";

This indent is wrong.

> Source/WebCore/dom/Document.cpp:2125
> +    // This method is called by various places in the WebCore code. Arguments
> +    // chosen for legacy reasons.

This comment is pretty vague.  Why else would a function exist other than to be called from various places?

> Source/WebCore/dom/Document.cpp:2163
>      if (m_frame)
> -        m_frame->loader()->didExplicitOpen();
> +        m_textWrittenByScript = SharedBuffer::create(); // Buffer to record data written by document.write calls.
> +        m_frame->loader()->didExplicitOpen(mimeType, replace, m_textWrittenByScript.get());

Did you mean to put { } around the body of this if-statement or did you mean for the second line to be executed unconditionally?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.