[webkit-reviews] review denied: [Bug 91231] WebHistoryItem: Enable reading selected file names from document state : [Attachment 157448] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Marja Hölttä
<marja at chromium.org>'s request for review:
Bug 91231: WebHistoryItem: Enable reading selected file names from document
state
https://bugs.webkit.org/show_bug.cgi?id=91231

Attachment 157448: Patch
https://bugs.webkit.org/attachment.cgi?id=157448&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157448&action=review


> Source/WebKit/chromium/src/WebHistoryItem.cpp:290
> +WebVector<WebString> WebHistoryItem::getReferredFilePaths() const

nit: getReferencedFilePaths().	getReferredFilePaths() sounds a bit awkward
to me.	did you have a reason for preferring "referred" here?  just because
it is shorter?

> Source/WebKit/chromium/src/WebHistoryItem.cpp:292
> +    WTF::Vector<WebString> filePaths;

nit: no need for WTF:: in .cpp files

> Source/WebKit/chromium/src/WebHistoryItem.cpp:295
> +	   WebHTTPBody::Element element;

nit: prefer using WebCore types inside the implementation of WebKit.
There are hidden costs to using WebKit API here.

> Source/WebKit/chromium/src/WebHistoryItem.cpp:302
> +    const WebVector<WebString>& state = documentState();

it is a bit wasteful to allocate a WebVector here.  you should just
use Vector<String> as returned by m_private->documentState() to avoid
copying the array.

> Source/WebKit/chromium/src/WebHistoryItem.cpp:304
> +    while (i + 2 < state.size()) {

you should explain what these magic numbers are all about.

> Source/WebKit/chromium/src/WebHistoryItem.cpp:317
> +    WebVector<WebString> toReturn(filePaths.size());

nit: you can just do "return filePaths;" and it will automagically
create a WebVector<WebString> for you.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2353
> +void LayoutTestController::getReferredFiles(const CppArgumentList&
arguments, CppVariant* result)

can you do this by extending Internals.idl instead?  we try to avoid adding new
methods
on LayoutTestController (now TestRunner) when possible.