[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 #11 from Dinu Jacob <dinu.jacob at Nokia.com>  2012-03-15 08:58:36 PST ---
(In reply to comment #8)
> (From update of attachment 131918 [details])
> 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.
> 

The bug was logged for the crash reported. The original patch was fixing this but since we need a solution for both QGuiApplications as well as QApplications, this patch to replace the current QFileDialog implementation was implemented.

> > Source/WebKit2/ChangeLog:10
> > +        to set the qml component for file upload triggered by an input file element.
> 
> Nitpick: QML :-)
> 
Will change that
> > 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.
> 
Will fix it.
> > 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.
> 
Can change it to filePaths.

> > Tools/MiniBrowser/qt/MiniBrowser.qrc:13
> > +        <file>icons/titlebar.png</file>
> 
This is used as a border image.

> 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.
> 
Will fix it.

> > 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?
> 
Wanted to provide a separate titlebar.

> > Tools/MiniBrowser/qt/qml/FilePicker.qml:32
> > +    id: root
> 
> Change this to id: filePickerDialog instead. Root is too ambiguous.
> 
Will fix it.

> > Tools/MiniBrowser/qt/qml/FilePicker.qml:102
> > +        id: folderList
> 
> folderListView, maybe?
>
Sure.

> > Tools/MiniBrowser/qt/qml/FilePicker.qml:199
> > +
> 
> Nitpick: Lots of whitespace in the end :-)

Will remove those.

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