SSIA.
Created attachment 118363 [details] Patch
Comment on attachment 118363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review > Tools/ChangeLog:16 > + * MiniBrowser/qt/qml/TouchMockPoint.qml: Copied from Tools/MiniBrowser/qt/BrowserWindow.h. Pretty stupid comment :-) pls remove and add Added > Tools/MiniBrowser/qt/BrowserWindow.cpp:88 > + QHash<int, QQuickItem*>::iterator iter = m_touchMockPoints.find(id); we normally use the name "it" in webkit > Tools/MiniBrowser/qt/BrowserWindow.cpp:99 > + > + iter.value()->setX(position.x() / 2); > + iter.value()->setY(position.y() / 2); > + iter.value()->setOpacity(opacity); why not (*it)->set... > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:174 > + // Get rid of touch-points that are no longer valid + . at the end :-) > Tools/MiniBrowser/qt/qml/TouchMockPoint.qml:31 > + visible: true isn't that default?
Comment on attachment 118363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review > Tools/MiniBrowser/qt/BrowserWindow.cpp:86 > +void BrowserWindow::updateTouchMockPoint(int id, const QPointF& position, qreal opacity) updateMockTouchPoints >> Tools/MiniBrowser/qt/BrowserWindow.cpp:88 >> + QHash<int, QQuickItem*>::iterator iter = m_touchMockPoints.find(id); > > we normally use the name "it" in webkit Look up old touch points that you can re-use from the engine based on the id, instead of keeping a list. > Tools/MiniBrowser/qt/BrowserWindow.cpp:97 > + iter.value()->setX(position.x() / 2); set the x and y, and do the offsetting to center it in the qml >> Tools/MiniBrowser/qt/BrowserWindow.cpp:99 >> + iter.value()->setOpacity(opacity); > > why not (*it)->set... i'd rather see a "pressed" property that you set and unset based on the point being released or not, and you can then deal with how you hide it in the qml > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:163 > + do we ever have a situation without a browser window? > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:168 > + qreal opacity = 1.0; see comment about "pressed". we should not deal with opacity here, that's an implementation detail of the visual component
(In reply to comment #3) > (From update of attachment 118363 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review > > > Tools/MiniBrowser/qt/BrowserWindow.cpp:86 > > +void BrowserWindow::updateTouchMockPoint(int id, const QPointF& position, qreal opacity) > > updateMockTouchPoints Or, updateVisualMockTouchPoints
Created attachment 118382 [details] Patch
Created attachment 118383 [details] Patch
Comment on attachment 118383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118383&action=review > Tools/MiniBrowser/qt/BrowserWindow.cpp:107 > + foreach (QQuickItem* item, rootObject()->childItems()) { > + QVariant itemId = item->property("id"); > + > + if (itemId.toString() == mockTouchPointIdentifier) { > + mockTouchPointItem = item; > + break; > + } > + } Set the objectName instead of the id, and use rootObject()->findChild<QQuickItem*>(mockTouchPointIdentifier); > Tools/MiniBrowser/qt/BrowserWindow.cpp:118 > + mockTouchPointItem->setProperty("pressed", QVariant(pressed)); replace the pressed variable with touchPoint.state == Qt::TouchPointReleased > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:166 > + BrowserWindow* browserWindow = dynamic_cast<BrowserWindow*>(targetWindow); > + > + if (browserWindow) > + browserWindow->updateVisualMockTouchPoints(m_touchPoints.values()); > + if (BrowserWindow* browserWindow = qobject_cast<BrowserWindow*>(targetWindow)) browserWindow->updateVisualMockTouchPoints(m_touchPoints.values());
Created attachment 118393 [details] Patch
Comment on attachment 118393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118393&action=review > Tools/MiniBrowser/qt/BrowserWindow.cpp:92 > + bool pressed = touchPoint.state != Qt::TouchPointReleased; > + QPointF position = touchPoint.area.topLeft(); > + position.rx() -= geometry().x(); > + position.ry() -= geometry().y(); Move this down to where you set it, and remove the pressed-variable, you can set the property directly based on touchPoint.state != Qt::TouchPointReleased
Created attachment 118403 [details] Patch
Comment on attachment 118403 [details] Patch great!
Comment on attachment 118403 [details] Patch Clearing flags on attachment: 118403 Committed r102348: <http://trac.webkit.org/changeset/102348>
All reviewed patches have been landed. Closing bug.