[webkit-reviews] review denied: [Bug 14959] No back forward entry added for pages created in javascript : [Attachment 18052] updated patch

Sat Dec 29 15:14:35 PST 2007

Darin Adler <darin at apple.com> has denied Matt Perry <mpComplete at gmail.com>'s
request for review:
Bug 14959: No back forward entry added for pages created in javascript
http://bugs.webkit.org/show_bug.cgi?id=14959

Attachment 18052: updated patch
http://bugs.webkit.org/attachment.cgi?id=18052&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Patch looks great.

I have a few complaints that should be relatively simple to fix.

Many apologies for not reviewing this long ago. I had assumed that someone else
was going to review. Thank you very much for working on this.

 1311	  // Params set to match the old behavior, though replace=false is
probably a more sane default. 
 1312	  open("text/html", true);

I don't think the issue here is "sane default" but rather matching other
browsers, since open is a public DOM function accessible from JavaScript. Can
this change to replace = false? If not, then why not? This comment makes it
seem like something is broken. Is something broken here? If so, what? What test
would demonstrate the breakage?

 113	 String mimeType("text/html");
 114	 if (args.size() >= 1) {
 115	     mimeType = String(args[0]->toString(exec)).lower();
 116	     if (mimeType != "text/html" && mimeType != "replace")
 117		 mimeType = "text/plain";  // anything other than text/html is
treated as plaintext.
 118	 }

It's unnecessarily inefficient to actually allocate the buffer for the string
"text/html" -- the logic should be changed so that it gets set on both sides of
an if.

Generally, we do not ever check the size of arguments. Instead we rely on the
fact that arguments not passed are undefined. There are very few
counterexamples to this (although one of them is earlier in this very
function). And the JavaScript arguments list object makes this easy by
returning undefined for any values past the end of the list. So this code
should check args[0]->isUndefined() rather than args.size(). It's also worth
testing behavior of other browsers when the value null is passed in -- is this
treated as the MIME type "text/plain" or as "text/html"? It would be good to
have a few tests that cover this. The comment doesn't make it clear at all why
the value "replace" is allowed for mimeType and I must admit I don't understand
why. The comment should start with a capital letter if it ends with a period.

 120	 bool replace = false;
 121	 if (args.size() >= 2)
 122	     replace = equalIgnoringCase("replace",
String(args[1]->toString(exec)));

The best way to handle this is probably to just leave out the if statement. We
can let values like undefined get converted to string and rejected since they
aren't equal to "replace".

 920	 // Contains the text written to the document by script, eg through
document.write().
 921	 RefPtr<SharedBuffer> m_scriptedDocumentBuf;

How about a name that matches the comment more closely? Perhaps
m_textWrittenByScript.

I don't like the existing name because we don't normally abbreviate buffer as
"buf" and I don't see how "scripted document buffer" makes sense, since there
is no such thing as a scripted document.

 1578	      open(String("text/html"), false);

Why the explicit cast to String here?

1567	      write(DeprecatedString("<html>"));
 1582	      write(String("<html>"));

Can we get rid of the DeprecatedString overload?

 1587	  if (m_scriptedDocumentBuf) {
 1588	      CString utf8 = text.utf8();
 1589	      m_scriptedDocumentBuf->append(utf8.data(), utf8.length());
 1590	  }

Is there a good reason to convert this text to UTF-8 then decode it back to
UTF-16 later? We really don't want to be converting back and forth between
UTF-16 and UTF-8 unless there's a benefit to doing so. I'd like to avoid the
extra memory allocation too.

 350	 void open(const String&, bool);

While we do omit argument names in headers when they are not needed, this is a
case where they are needed. It's not at all clear what the string or boolean
are. We leave the names out when the argument type or function name makes the
name superfluous.

 740	 bool newItem = false;

The boolean should not be named "new item" because it is a boolean flag, not an
item. It should be named something like isItemNew, itemIsNew or
historyItemIsNew.

 755	 DeprecatedString generatedURLString("generated:");
 756	 generatedURLString.append(m_frame->document()->url());
 757	 KURL generatedURL(generatedURLString);

For code like this, it's generally better to not make a temporary -- for one
thing it means we have to revisit this callsite when eliminating
DeprecatedString. This should simply be:

    KURL generatedURL("generated:" + m_frame->document()->url());

Perhaps the URL scheme should also be changed to something with a vendor
prefix. Just plain "generated:" seems like it might conflict with other uses.
Something with webkit in it would probably be better. Can the URL used here be
detected by JavaScript? If so, what effect will the URL have on site
compatibility? What URL do you see on other web browsers in this case? Do any
of the tests detect this generated URL?

 759	 SubstituteData substituteData(buffer, mimeType, String("UTF-8"),
m_URL, generatedURL);
 760	 item->setSubstituteData(substituteData);

This should use UTF-16 and we should not be converting text to UTF-8 at all.
And I think there's no need to have a named temporary here.

20822108 void FrameLoader::load(const ResourceRequest& request, const
NavigationAction& action, FrameLoadType type, PassRefPtr<FormState> formState)

 2113 void FrameLoader::load(const ResourceRequest& request, const
NavigationAction& action, FrameLoadType type, PassRefPtr<FormState> formState,
const SubstituteData& substituteData)

We're really concerned about the huge number of load functions in the
FrameLoader class and we're trying to fix that. Adding yet another is
unfortunate. On the other hand, since this effectively just gives the last
parameter a default value of null, I think it's OK.