Bug 92001 – [EFL][WK2] Update uri when the active URI is changed while loading.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

For now, ewk_view_uri_get just return the last uri of setter.
Because uri can be changed while loading, the uri of ewk_view should be updated.

[Ryuan Choi]

Created attachment 153800 [details]
Patch

[Chris Dumez]

Comment on attachment 153800 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:548
> +    if (priv->uri && activeURI == priv->uri)

This check becomes useless if we use the output of eina_stringshare_replace. See below.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:551
> +    eina_stringshare_replace(&priv->uri, activeURI.data());

eina_stringshare_replace returns a boolean. You can do:
if (!eina_stringshare_replace(&priv->uri, activeURI.data()))
    return;

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:27
> +#define TEST_PAGE_URI(html) ("file://"TEST_RESOURCES_DIR"/"html)

How about using a static method in EWK2UnitTestBase instead? Also, I think the name should be something like "pathForResource(const char* resource);". The Resource may not be an HTML page, it may be an icon, a pdf file, ...

[Ryuan Choi]

Created attachment 153884 [details]
Patch

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 153800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153800&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:548
> > +    if (priv->uri && activeURI == priv->uri)
> 
> This check becomes useless if we use the output of eina_stringshare_replace. See below.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:551
> > +    eina_stringshare_replace(&priv->uri, activeURI.data());
> 
> eina_stringshare_replace returns a boolean. You can do:
> if (!eina_stringshare_replace(&priv->uri, activeURI.data()))
>     return;
> 
I fixed.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:27
> > +#define TEST_PAGE_URI(html) ("file://"TEST_RESOURCES_DIR"/"html)
> 
> How about using a static method in EWK2UnitTestBase instead? Also, I think the name should be something like "pathForResource(const char* resource);". The Resource may not be an HTML page, it may be an icon, a pdf file, ...

I agree that pathForResource is better name.
And I used macro to avoid string manipulation and free, but it's not important.
I added pathForResource like you mentioned.

thank you for your comment.

[Chris Dumez]

Comment on attachment 153884 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:548
> +    if (eina_stringshare_replace(&priv->uri, activeURI.data()))

Eh... Isn't it the opposite? :)

I mean: if (!eina_stringshare_replace()) return;

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:42
> +Eina_Stringshare* EWK2UnitTestEnvironment::pathForResource(const char* resource)

I would return a CString to avoid handling memory manually. CString is always commonly used in EFL tests.

[Ryuan Choi]

Created attachment 153981 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> (From update of attachment 153884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153884&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:548
> > +    if (eina_stringshare_replace(&priv->uri, activeURI.data()))
> 
> Eh... Isn't it the opposite? :)
> 
> I mean: if (!eina_stringshare_replace()) return;
> 
Ah.. sorry, my mistake.
I fixed.
Thank you.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:42
> > +Eina_Stringshare* EWK2UnitTestEnvironment::pathForResource(const char* resource)
> 
> I would return a CString to avoid handling memory manually. CString is always commonly used in EFL tests.

Done.

[Chris Dumez]

Comment on attachment 153981 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:46
> +    return makeString("file://"TEST_RESOURCES_DIR"/", resource).utf8();

If you prepend "file://" then the method name is wrong. This returns a URL, not a path.

[Ryuan Choi]

Created attachment 153986 [details]
Patch

[Chris Dumez]

Comment on attachment 153986 [details]
Patch

LGTM.

[Hajime Morrita]

Comment on attachment 153986 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 153986 [details]
Patch

Clearing flags on attachment: 153986

Committed r123591: <http://trac.webkit.org/changeset/123591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.