Bug 72910

Created attachment 116129 [details] Patch

Comment on attachment 116129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review Love it! r=me with some nitpicking. > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:115 > + // FIXME: Remove scoped pointer when possible. When will this be possible? > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:116 > + QScopedPointer<QtWebPageUIClient> m_pageUIClient; Per Qt API style, this member should not have the "m_" prefix. > Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:95 > + if (!wkSelectedFileNames.isEmpty()) This check can be removed. > Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96 > + for (unsigned i = 0; wkSelectedFileNames.size(); ++i) Vector::size() returns size_t, not unsigned. > Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:39 > + // WKPageUIClient callbacks. Let's make these private instead of public. > Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:43 > + static void setStatusText(WKPageRef, WKStringRef text, const void *clientInfo); Nit: The 'text' argument name is unnecessary. > Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:49 > + void runJavaScriptAlert(const QString& alertText); > + bool runJavaScriptConfirm(const QString& message); > + QString runJavaScriptPrompt(const QString& message, const QString& defaultValue, bool& ok); Nit: The 'alertText' argument name should probably also be called 'message'.

Comment on attachment 116129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96 >> + for (unsigned i = 0; wkSelectedFileNames.size(); ++i) > > Vector::size() returns size_t, not unsigned. I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()".

Created attachment 116175 [details] Patch

Comment on attachment 116129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review >> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:115 >> + // FIXME: Remove scoped pointer when possible. > > When will this be possible? I had the plan of making this object directly a member instead of (smart)pointer. For this to happen, we need to make sure we can create the QtWebPageProxy in the initalization list. But this is an unrelated change, let's not do it now. >> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:116 >> + QScopedPointer<QtWebPageUIClient> m_pageUIClient; > > Per Qt API style, this member should not have the "m_" prefix. Fixed. >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:95 >> + if (!wkSelectedFileNames.isEmpty()) > > This check can be removed. Fixed. Note that this and next comment refer to code in functions that were just moved from QtWebPageProxy. I fixed them anyway. >>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96 >>> + for (unsigned i = 0; wkSelectedFileNames.size(); ++i) >> >> Vector::size() returns size_t, not unsigned. > > I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()". Fixed the size_t and the bug, nice catch Rafael :-) >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:39 >> + // WKPageUIClient callbacks. > > Let's make these private instead of public. Fixed. >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:43 >> + static void setStatusText(WKPageRef, WKStringRef text, const void *clientInfo); > > Nit: The 'text' argument name is unnecessary. Fixed. >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:49 >> + QString runJavaScriptPrompt(const QString& message, const QString& defaultValue, bool& ok); > > Nit: The 'alertText' argument name should probably also be called 'message'. Fixed.

Created attachment 116181 [details] Patch

(In reply to comment #3) > (From update of attachment 116129 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review > > >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96 > >> + for (unsigned i = 0; wkSelectedFileNames.size(); ++i) > > > > Vector::size() returns size_t, not unsigned. > > I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()". I remembered you ran the API tests. We do have an awesome coverage :D. ahaha

(In reply to comment #7) > I remembered you ran the API tests. We do have an awesome coverage :D. ahaha This will be easier to test when we move to QML for the file selection dialog. Stay tuned ;)

Comment on attachment 116181 [details] Patch Woosh!

Committed r100990: <http://trac.webkit.org/changeset/100990>