[Webkit-unassigned] [Bug 80854] [Qt][Wk2] Assertion Failure and crash on file upload

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Alexander Færøy <ahf at 0x90.dk>  2012-03-15 07:23:40 PST ---
(From update of attachment 131918)
View in context: https://bugs.webkit.org/attachment.cgi?id=131918&action=review

Why does it contain a black box for the titlebar.png?

> Source/WebKit2/ChangeLog:3
> +        [Qt][Wk2] Assertion Failure and crash on file upload

I don't think this name is descriptive enough. This patch is implementing the API for having a file picker and the bug name should probably reflect this.

> Source/WebKit2/ChangeLog:10
> +        to set the qml component for file upload triggered by an input file element.

Nitpick: QML :-)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:78
> +    , filePicker(0)

Use filePickerDialog for consistency.

> Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:178
> +    void accept(const QVariant& path) { emit fileSelected((path.toStringList())); }

Why the extra set of parentheses?

> Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:267
> +    connect(contextObject, SIGNAL(fileSelected(const QStringList&)), SLOT(onFileSelected(const QStringList&)));

No need to use reference-to-const in slots.

> Source/WebKit2/UIProcess/qt/QtDialogRunner.h:55
> +    QStringList filePath() const { return m_filepath; }

Not sure that I like this name to be on singular form. It is returning a QStringList and not just a single QString.

> Tools/MiniBrowser/qt/MiniBrowser.qrc:13
> +        <file>icons/titlebar.png</file>

Nitpick: We try to keep this list sorted.

> Tools/MiniBrowser/qt/MiniBrowser.qrc:21
> +        <file>qml/FilePicker.qml</file>

Again, FilePickerDialog.qml.

> Tools/MiniBrowser/qt/qml/FilePicker.qml:15
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

Looks incorrect to me, but we are very bad and this in general.

> Tools/MiniBrowser/qt/qml/FilePicker.qml:29
> +import Qt.labs.folderlistmodel 1.0

I am not sure about this at all, but is this safe enough to use and wont add any new dependencies?

> Tools/MiniBrowser/qt/qml/FilePicker.qml:31
> +Rectangle {

Any reason why you can't make this work with the already implemented Dialog QML type?

> Tools/MiniBrowser/qt/qml/FilePicker.qml:32
> +    id: root

Change this to id: filePickerDialog instead. Root is too ambiguous.

> Tools/MiniBrowser/qt/qml/FilePicker.qml:102
> +        id: folderList

folderListView, maybe?

> Tools/MiniBrowser/qt/qml/FilePicker.qml:199
> +

Nitpick: Lots of whitespace in the end :-)

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