Added signal to let applications know contents size. Applications can use this signal to give some additional behavior such as minimap, external scroll.
Created attachment 175719 [details] Patch
Please add the callback description in ewk_view.h
Created attachment 175721 [details] Patch
Comment on attachment 175721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175721&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:-728 > - m_pageClient->didChangeContentsSize(size); how did you come to removing it? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:727 > + int contentsSize[2]; Think it's really better to have a size structure > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:456 > +#if PLATFORM(QT) || PLATFORM(EFL) why not use #if USE(TILED_BACKING_STORE) here as well?
Comment on attachment 175721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175721&action=review >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:-728 >> - m_pageClient->didChangeContentsSize(size); > > how did you come to removing it? Odd indeed. >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:727 >> + int contentsSize[2]; > > Think it's really better to have a size structure I definitely agree. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:893 > +TEST_F(EWK2UnitTestBase, ewk_view_contnets_size_changed) Typo "contnets"
(In reply to comment #4) > (From update of attachment 175721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175721&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:-728 > > - m_pageClient->didChangeContentsSize(size); > > how did you come to removing it? > After previous refactoring, informContentsSizeChange was only called by PageClientDefaultImpl and PageClientLegacyImpl when TILED_BACKING_STORE is disabled. So, I believe that this was dead code. > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:727 > > + int contentsSize[2]; > > Think it's really better to have a size structure > I will change to Evas_Coord_Size. > > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:456 > > +#if PLATFORM(QT) || PLATFORM(EFL) > > why not use #if USE(TILED_BACKING_STORE) here as well? I want to send DidChangeContentsSize although TILED_BACKINGSTORE was disabled. (In reply to comment #5) > (From update of attachment 175721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175721&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:-728 > >> - m_pageClient->didChangeContentsSize(size); > > > > how did you come to removing it? > > Odd indeed. > > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:727 > >> + int contentsSize[2]; > > > > Think it's really better to have a size structure > > I definitely agree. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:893 > > +TEST_F(EWK2UnitTestBase, ewk_view_contnets_size_changed) > > Typo "contnets" Oops, I will fix it. Thanks.
Created attachment 175906 [details] Patch
(In reply to comment #7) > Created an attachment (id=175906) [details] > Patch Applied above comments. Exceptionally, I created Ewk_Size temporarily instead of Evas_Coord_Size because Evas_Coord_Size is introduced after EFL 1.7.
Comment on attachment 175906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175906&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:284 > + Evas_Coord w; /**< x co-ordinate */ comment looks wrong. Not x co-ordinate, but width value. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:285 > + Evas_Coord h; /**< y co-ordinate */ ditto.
Created attachment 175909 [details] Patch
Created attachment 176742 [details] Patch
(In reply to comment #11) > Created an attachment (id=176742) [details] > Patch rebase because of email. Christophe, Mikhail, Could you give me an opinion?
Comment on attachment 176742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:903 > + while (sizeChanged) Am I misreading? Shouldn't is be "while (!sizeChanged)" ?
Comment on attachment 176742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:737 > void EwkViewImpl::informContentsSizeChange(const IntSize& size) I don't think EwkView needs this method at all. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:740 > + Ewk_Size contentsSize; Ewk_Size contentsSize { size.width(), size.height() }, just more compact
Comment on attachment 176742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:448 > +#if USE(TILED_BACKING_STORE) Are you sure QT port doesn't need to use this code block when TILED_BACKING_STORE is disabled ?
Created attachment 176893 [details] Patch
(In reply to comment #13) > (From update of attachment 176742 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:903 > > + while (sizeChanged) > > Am I misreading? Shouldn't is be "while (!sizeChanged)" ? Oops. you're right. fixed. (In reply to comment #14) > (From update of attachment 176742 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:737 > > void EwkViewImpl::informContentsSizeChange(const IntSize& size) > > I don't think EwkView needs this method at all. > I thought smartcall should be in EwkView. I removed it. > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:740 > > + Ewk_Size contentsSize; > > Ewk_Size contentsSize { size.width(), size.height() }, just more compact Done. (In reply to comment #15) > (From update of attachment 176742 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176742&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:448 > > +#if USE(TILED_BACKING_STORE) > > Are you sure QT port doesn't need to use this code block when TILED_BACKING_STORE is disabled ? I heard like that and if disabled TILED_BACKINGSTORE in qt port, previous code make build break because this code is only available when TILED BACKINGSTORE is enabled.
Comment on attachment 176893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176893&action=review > Source/WebKit2/ChangeLog:23 > + Sended DidChangeContentsSize message although TILED_BACKINGSTORE is disabled. It would be good if you say QT port also be influenced on this patch.
Created attachment 176901 [details] Patch
(In reply to comment #18) > (From update of attachment 176893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176893&action=review > > > Source/WebKit2/ChangeLog:23 > > + Sended DidChangeContentsSize message although TILED_BACKINGSTORE is disabled. > > It would be good if you say QT port also be influenced on this patch. As discussed, I changed patch to match with comment exactly. Thank you.
Comment on attachment 176893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176893&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:448 > +#if USE(TILED_BACKING_STORE) Kenneth, I wonder if QT port can accept this change. Could you check this ?
> Kenneth, I wonder if QT port can accept this change. Could you check this ? Adding Jocelyn
Comment on attachment 176901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176901&action=review > Source/WebKit2/ChangeLog:11 > + > + Reviewed by NOBODY (OOPS!). > + > + This patch emits signal to let applications know contents size. > + > + * UIProcess/API/efl/EwkViewCallbacks.h: Added contents,size,changed signal. > + * UIProcess/API/efl/EwkViewImpl.cpp: Removed dead code. You don't explain why apps need to know about this > Source/WebKit2/UIProcess/API/efl/ewk_view.h:286 > + > +/** > + * @brief Structure representing size. > + * @info This structure will be deprecated after EFL 1.8 > + */ > +struct Ewk_Size { > + Evas_Coord w; /**< width */ > + Evas_Coord h; /**< height */ > +}; So now the question is. Contents size? Is that in css units? device units? Documentation should mention this. Did you test if deviceScaleFactor != 1, say == 2 ? > Source/WebKit2/UIProcess/efl/PageClientDefaultImpl.cpp:102 > + // FIXME: It should be changed to Evas_Coord_Size after EFL 1.8 > + Ewk_Size contentsSize = { size.width(), size.height() }; > + m_viewImpl->smartCallback<ContentsSizeChanged>().call(&contentsSize); bug link? Also, why is that more clear? Evas_Coord_Size makes it think this is in UI units. I think even a Ewk_CSS_Size or similar would be more clear, ofcourse such a thing could be a typedef of Evas_Coord_Size if that makes any sense
Comment on attachment 176893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176893&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:448 >> +#if USE(TILED_BACKING_STORE) > > Kenneth, I wonder if QT port can accept this change. Could you check this ? It's fine to me, fixed layout code also depend on TILED_BACKING_STORE for us.
Created attachment 177244 [details] Patch
(In reply to comment #23) > (From update of attachment 176901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176901&action=review > > > Source/WebKit2/ChangeLog:11 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + This patch emits signal to let applications know contents size. > > + > > + * UIProcess/API/efl/EwkViewCallbacks.h: Added contents,size,changed signal. > > + * UIProcess/API/efl/EwkViewImpl.cpp: Removed dead code. > > You don't explain why apps need to know about this > I added well-known simple use case. > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:286 > > + > > +/** > > + * @brief Structure representing size. > > + * @info This structure will be deprecated after EFL 1.8 > > + */ > > +struct Ewk_Size { > > + Evas_Coord w; /**< width */ > > + Evas_Coord h; /**< height */ > > +}; > > So now the question is. Contents size? Is that in css units? device units? Documentation should mention this. Did you test if deviceScaleFactor != 1, say == 2 ? > I updated test cases. In addition, I changed Ewk_Size to Ewk_CSS_Size It looks better like you mentioned. > > Source/WebKit2/UIProcess/efl/PageClientDefaultImpl.cpp:102 > > + // FIXME: It should be changed to Evas_Coord_Size after EFL 1.8 > > + Ewk_Size contentsSize = { size.width(), size.height() }; > > + m_viewImpl->smartCallback<ContentsSizeChanged>().call(&contentsSize); > > bug link? Also, why is that more clear? Evas_Coord_Size makes it think this is in UI units. I think even a Ewk_CSS_Size or similar would be more clear, ofcourse such a thing could be a typedef of Evas_Coord_Size if that makes any sense I thought that it's better to use primilary types. But, as you mentioned, Ewk_CSS_Size can be typedef when it is available. So I removed FIXME. Thank you.
Comment on attachment 177244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177244&action=review > Source/WebKit2/UIProcess/efl/PageClientDefaultImpl.cpp:100 > + Ewk_CSS_Size contentsSize = { size.width(), size.height() }; how about adding one more template specialization like CallBack <callbackType, Ewk_CSS_Size*> it could contain 'call(const WebCore::IntSize& size)' and do the conversion inside
Created attachment 177253 [details] Patch
(In reply to comment #27) > (From update of attachment 177244 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177244&action=review > > > Source/WebKit2/UIProcess/efl/PageClientDefaultImpl.cpp:100 > > + Ewk_CSS_Size contentsSize = { size.width(), size.height() }; > > how about adding one more template specialization like CallBack <callbackType, Ewk_CSS_Size*> > it could contain 'call(const WebCore::IntSize& size)' and do the conversion inside OK. I tried like you mentioned.
Comment on attachment 177253 [details] Patch LGTM, thanks
Committed r136402: <http://trac.webkit.org/changeset/136402>
Comment on attachment 177253 [details] Patch thank you