[webkit-reviews] review granted: [Bug 7872] Unleash wrath upon DeprecatedString : [Attachment 7186] Kill lots of DeprecatedString

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 7872: Unleash wrath upon DeprecatedString
http://bugzilla.opendarwin.org/show_bug.cgi?id=7872

Attachment 7186: Kill lots of DeprecatedString
http://bugzilla.opendarwin.org/attachment.cgi?id=7186&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    return jsString(userAgent.deprecatedString().mid(userAgent.find('/') +
1));

You can use String::substring for this; no need to convert back to
DeprecatedString just to call DeprecatedString::mid.

-DeprecatedString TransferJob::errorText() const
+String TransferJob::errorText() const

Can't we just remove this unused function?

+	     const AtomicString &stdfont = settings->stdFontName();
+	 if (DocumentType *doctype = doc->realDocType())
+void FrameView::setMediaType(const String &mediaType)
+	 void setContentType(const String &t) { m_contentType = t; }

Please put the & or * by the type name and not the variable name.

+    return [[(NSString*)text copy] autorelease];

Using a local variable is slightly nicer in these cases, because then you don't
have to use the cast syntax. The cast is a bit too powerful, since you can pass
a pointer of the wrong type and it will just convert the pointer, so it's a
nicer style to not have casts when they can be avoided.

+	[bridge impl]->tree()->setName(AtomicString(request.frameName));

After my patch you would not need the AtomicString constructor here (the one
you were looking at but didn't finish removing).

+  unsigned highlightAllMatchesForString(const String& , bool caseFlag);

Please omit the space after the & here.

-    int _width;
-    int _height;
-
-    int _marginWidth;
-    int _marginHeight;
+    IntSize m_size;
+    IntSize m_margins;
 
     RefPtr<Frame> m_frame;
     FrameViewPrivate* d;
 
-    DeprecatedString m_medium; // media type
+    String m_mediaType;

Shouldn't all of these go into FrameViewPrivate?