Bug 102122 – [EFL][WK2] New window size should consult the window attributes

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Yael]

Currently, new window is always sized to 800x600, and only later resized to the desired size.
This can lead to ugly flashing.
Patch is coming.

[Yael]

Created attachment 173962 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 173962 [details]
Patch

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

Nice!

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:841
> +    WKRetainPtr<WKStringRef> widthStr(AdoptWK, WKStringCreateWithUTF8CString("width"));
> +    WKRetainPtr<WKStringRef> heightStr(AdoptWK, WKStringCreateWithUTF8CString("height"));
> +

Maybe a comment that we don't do everything here? for showModalDialog are they also called width and height or dialogWidth etc?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:842
> +    int width = 0;

unsigned?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:846
> +    if (ref)
> +        width = WKDoubleGetValue(static_cast<WKDoubleRef>(ref));

unsigned width = ref ? WKDoubleGetValue(static_cast<WKDoubleRef>(ref)) : 0 ?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:852
> +    width = std::max(width, 0);
> +    height = std::max(height, 0);

WebKit makes sure these are always bigger than 100px. You could assert

[Yael]

(In reply to comment #2)
> (From update of attachment 173962 [details])
Thanks for the review :)
> View in context: https://bugs.webkit.org/attachment.cgi?id=173962&action=review
> 
> Nice!
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:841
> > +    WKRetainPtr<WKStringRef> widthStr(AdoptWK, WKStringCreateWithUTF8CString("width"));
> > +    WKRetainPtr<WKStringRef> heightStr(AdoptWK, WKStringCreateWithUTF8CString("height"));
> > +
> 
> Maybe a comment that we don't do everything here? for showModalDialog are they also called width and height or dialogWidth etc?
> 
The dictionary is created in WebUIClient::createNewPage(), and always uses "width" and "height".
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:842
> > +    int width = 0;
> 
> unsigned?
> 
ok
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:846
> > +    if (ref)
> > +        width = WKDoubleGetValue(static_cast<WKDoubleRef>(ref));
> 
> unsigned width = ref ? WKDoubleGetValue(static_cast<WKDoubleRef>(ref)) : 0 ?
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:852
> > +    width = std::max(width, 0);
> > +    height = std::max(height, 0);
> 
> WebKit makes sure these are always bigger than 100px. You could assert
WebKit does that only if a value was specified. But you are right, I will never get a negative value here.

[Yael]

Created attachment 173996 [details]
Patch

Address comment #2.

[Kenneth Rohde Christiansen]

Comment on attachment 173996 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:844
> +    unsigned int width = ref ? WKDoubleGetValue(static_cast<WKDoubleRef>(ref)) : 0;

style says to just use "unsigned" ie. no int

> Tools/MiniBrowser/efl/main.c:187
> -        Browser_Window *window = window_create(DEFAULT_URL);
> +        Browser_Window *window = window_create(DEFAULT_URL, 0, 0);

// 0 equals default.

[Yael]

Created attachment 174019 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 174019 [details]
Patch for landing.

Clearing flags on attachment: 174019

Committed r134534: <http://trac.webkit.org/changeset/134534>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Shouldn't this patch take into consideration the window position as well? Or is it OK to move the window later on?

[Jinwoo Song]

Actually, I'm working on window features and will upload the patch for review soon.
https://bugs.webkit.org/show_bug.cgi?id=99114

[Yael]

(In reply to comment #9)
> Shouldn't this patch take into consideration the window position as well? Or is it OK to move the window later on?
Could be done in a separate patch, it seems that someone else is already working on it.