Bug 103094 – [EFL][WK2] Add contents,size,changed signal to the ewk_view API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Added signal to let applications know contents size.
Applications can use this signal to give some additional behavior such as minimap, external scroll.

[Ryuan Choi]

Created attachment 175719 [details]
Patch

[Jinwoo Song]

Please add the callback description in ewk_view.h

[Ryuan Choi]

Created attachment 175721 [details]
Patch

[Mikhail Pozdnyakov]

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?

[Chris Dumez]

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"

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 175906 [details]
Patch

[Ryuan Choi]

(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.

[Jinwoo Song]

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.

[Ryuan Choi]

Created attachment 175909 [details]
Patch

[Ryuan Choi]

Created attachment 176742 [details]
Patch

[Ryuan Choi]

(In reply to comment #11)
> Created an attachment (id=176742) [details]
> Patch

rebase because of email.

Christophe, Mikhail,

Could you give me an opinion?

[Chris Dumez]

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)" ?

[Mikhail Pozdnyakov]

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

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

Created attachment 176893 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 176901 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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 Rohde Christiansen]

> Kenneth, I wonder if QT port can accept this change. Could you check this ?

Adding Jocelyn

[Kenneth Rohde Christiansen]

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

[Jocelyn Turcotte]

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.

[Ryuan Choi]

Created attachment 177244 [details]
Patch

[Ryuan Choi]

(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.

[Mikhail Pozdnyakov]

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

[Ryuan Choi]

Created attachment 177253 [details]
Patch

[Ryuan Choi]

(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.

[Mikhail Pozdnyakov]

Comment on attachment 177253 [details]
Patch

LGTM, thanks

[Ryuan Choi]

Committed r136402: <http://trac.webkit.org/changeset/136402>

[Ryuan Choi]

Comment on attachment 177253 [details]
Patch

thank you