Bug 93100 - Remove minimum window size for PagePopup
Summary: Remove minimum window size for PagePopup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-03 05:38 PDT by Keishi Hattori
Modified: 2012-08-06 04:38 PDT (History)
2 users (show)

See Also:

Attachments
Patch (28.91 KB, patch)
2012-08-03 06:03 PDT, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (27.10 KB, patch)
2012-08-05 23:18 PDT, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (24.89 KB, patch)
2012-08-06 00:12 PDT, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-08-03 05:38:41 PDT 
Right now PagePopup cannot be resized below 100x100 pixels.
Comment 1 Keishi Hattori 2012-08-03 06:03:02 PDT 
Created attachment 156343 [details]
Patch
Comment 2 Kent Tamura 2012-08-03 07:01:12 PDT 
Comment on attachment 156343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156343&action=review

> Source/WebCore/page/Chrome.cpp:567
> +#if ENABLE(PAGE_POPUP)
> +    if (DOMWindowPagePopup::from(window))
> +        return FloatSize(0, 0);
> +#endif
> +    return FloatSize(100, 100);

I feel this is a violation for the concept of Supplement. Checking existence of a Supplement in a class unrelated to the Supplement looks wrong.

We should
 - Add minimumSizeForWindow to ChromeClient.  Not Chrome.
  I think its's ok that ChromeClient.h has a default implementation which reutrns FloatSize(100,100).
 - PagePopupChromeClient in WebPagePopupImpl.cpp overrides it.
Comment 3 Keishi Hattori 2012-08-05 23:18:30 PDT 
Created attachment 156602 [details]
Patch
Comment 4 Kent Tamura 2012-08-05 23:23:20 PDT 
Comment on attachment 156602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156602&action=review

> Source/WebCore/page/Chrome.cpp:61
> +#if ENABLE(PAGE_POPUP)
> +#include "DOMWindowPagePopup.h"
> +#endif
> +

unnecessary change

> Source/WebCore/page/Chrome.h:44
> +    class DOMWindow;

unnecessary change

> Source/WebCore/page/ChromeClient.h:354
> +        virtual FloatSize minimumSizeForWindow() const { return FloatSize(100, 100); };

nit: I prefer 'minimumWindowSize'.

> Source/WebCore/page/DOMWindow.cpp:342
> +    FloatSize minimumSize = page ? m_frame->page()->chrome()->minimumSizeForWindow() : FloatSize(100, 100);

We can call ChromeClient::minimumSizeForWindow() here.  So we don't need to change Chrome.{cpp.h} in this patch.

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:126
> +    virtual FloatSize minimumSizeForWindow() const

Please add OVERRIDE.

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:212
> +    ASSERT(frame->existingDOMWindow());
> +    DOMWindowPagePopup::install(frame->existingDOMWindow(), m_popupClient);
> +

This change is unnecessary.
Comment 5 Keishi Hattori 2012-08-06 00:12:22 PDT 
Created attachment 156612 [details]
Patch
Comment 6 Kent Tamura 2012-08-06 00:17:30 PDT 
Comment on attachment 156612 [details]
Patch

ok
Comment 7 WebKit Review Bot 2012-08-06 04:38:07 PDT 
Comment on attachment 156612 [details]
Patch

Clearing flags on attachment: 156612

Committed r124753: <http://trac.webkit.org/changeset/124753>
Comment 8 WebKit Review Bot 2012-08-06 04:38:10 PDT 
All reviewed patches have been landed.  Closing bug.