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

[bugzilla-daemon at webkit.org]

jochen 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 158251: Patch
https://bugs.webkit.org/attachment.cgi?id=158251&action=review

------- Additional Comments from jochen at chromium.org
View in context: https://bugs.webkit.org/attachment.cgi?id=158251&action=review


> Source/WebCore/ChangeLog:18
> +	   (WebCore::FormController::getReferencedFilePaths):

please add a comment to the newly added methods explaining what they do

> Source/WebCore/html/FormController.cpp:81
> +    for (size_t i = 0; i < m_values.size(); i += 2) {

this seems to assume that the FormControlState is representing a file input
which is not necessarily true. I think I'd prefer a static function that does
the magic of getting the filepaths out of the form control state. It could be
something like getReferencedFilePathsForFileFormControlState(FormControlState&)
or so

> Source/WebCore/html/FormController.cpp:287
> +	       const Vector<String>& files =
queIterator->getReferencedFilePaths();

just toReturn.append(queIterator->getReferencedFilePaths())

> Source/WebCore/html/FormController.cpp:522
> +	   const Vector<String>& filePaths = state->getReferencedFilePaths();

just toReturn.append(state->getReferencedFilePaths())

> Source/WebCore/testing/Internals.cpp:1198
> +    for (size_t i = 0; i < filePaths.size(); ++i)

you could Vector<String>& result = stringList; result.append(filePaths)