[webkit-reviews] review denied: [Bug 75471] [Qt] Add UI for JavaScript Alert dialog in the Qt MiniBrowser : [Attachment 120941] Patch

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Alexander Færøy <ahf at 0x90.dk>'s
request for review:
Bug 75471: [Qt] Add UI for JavaScript Alert dialog in the Qt MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=75471

Attachment 120941: Patch
https://bugs.webkit.org/attachment.cgi?id=120941&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120941&action=review


Looks good, but a few comments before landing

> Tools/MiniBrowser/qt/qml/Dialog.qml:58
> +	   clip: true

Do we need to clip it?

> Tools/MiniBrowser/qt/qml/Dialog.qml:64
> +	       font.weight: Font.Bold

Width, and eliding, so that we don't have to clip?

> Tools/MiniBrowser/qt/qml/Dialog.qml:69
> +	       anchors.centerIn: parent

Should this have a width and a wrap mode?

> Tools/MiniBrowser/qt/qml/Dialog.qml:76
> +	       anchors.margins: 10
> +	       anchors.bottom: staticContent.bottom
> +	       anchors.horizontalCenter: staticContent.horizontalCenter

I prefer anchors {} when there are more than one property

> Tools/MiniBrowser/qt/qml/DialogButton.qml:37
> +    color: "#fbfbfb"

color: mouseArea.pressed ? ....

> Tools/MiniBrowser/qt/qml/DialogButton.qml:57
> +	   onPressed: button.color = "#eaeaea"

You can remove this and use a binding (above)