[webkit-reviews] review denied: [Bug 98499] Page popup should be smarter about its layout : [Attachment 167307] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 98499: Page popup should be smarter about its layout
https://bugs.webkit.org/show_bug.cgi?id=98499

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167307&action=review


> Source/WebCore/Resources/pagepopups/pickerCommon.js:102
> +    // Make vertical adjustments

Such "what we do" comment indicates that we had better split the code in this
function into multiple functions.

> Source/WebCore/Resources/pagepopups/pickerCommon.js:122
> +    // Make horizontal adjustments

ditto.

> Source/WebCore/page/PagePopupClient.cpp:95
> +void PagePopupClient::addProperty(const char* name, IntRect rect,
DocumentWriter& writer)

IntRect -> const IntRect&

> Source/WebCore/page/PagePopupClient.cpp:102
> +    addProperty("x", static_cast<unsigned>(rect.x()), writer);
> +    addProperty("y", static_cast<unsigned>(rect.y()), writer);
> +    addProperty("width", static_cast<unsigned>(rect.width()), writer);
> +    addProperty("height", static_cast<unsigned>(rect.height()), writer);

We should have addProperty for int.

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:111
> +    WebCore::IntRect anchorRectInScreen =
m_chromeClient->rootViewToScreen(m_client->elementRectRelativeToRootView());
> +    WebCore::FrameView* view =
static_cast<WebViewImpl*>(m_chromeClient->webView())->page()->mainFrame()->view
();
> +    WebCore::IntRect rootViewVisibleContentRect =
view->visibleContentRect(true /* include scrollbars */);
> +    WebCore::IntRect rootViewRectInScreen =
m_chromeClient->rootViewToScreen(rootViewVisibleContentRect);

You may add "using namespace WebCore"

> LayoutTests/ChangeLog:9
> +	   *
platform/chromium/fast/forms/date/page-popup-adjust-rect-expected.txt: Added.
> +	   * platform/chromium/fast/forms/date/page-popup-adjust-rect.html:
Added.

Putting this test to fast/forms/date/ looks strange. We should make
fast/forms/calendar-picker/ or fast/forms/page-popup/ in order that Android
port can skip it easily.