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.
Created attachment 173962 [details] Patch
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
(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.
Created attachment 173996 [details] Patch Address comment #2.
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.
Created attachment 174019 [details] Patch for landing.
Comment on attachment 174019 [details] Patch for landing. Clearing flags on attachment: 174019 Committed r134534: <http://trac.webkit.org/changeset/134534>
All reviewed patches have been landed. Closing bug.
Shouldn't this patch take into consideration the window position as well? Or is it OK to move the window later on?
Actually, I'm working on window features and will upload the patch for review soon. https://bugs.webkit.org/show_bug.cgi?id=99114
(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.