Bug 92001

Summary: [EFL][WK2] Update uri when the active URI is changed while loading.
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2012-07-23 07:47:12 PDT
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.
Comment 1 Ryuan Choi 2012-07-23 08:13:09 PDT
Created attachment 153800 [details]
Patch
Comment 2 Chris Dumez 2012-07-23 08:34:38 PDT
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, ...
Comment 3 Ryuan Choi 2012-07-23 15:48:14 PDT
Created attachment 153884 [details]
Patch
Comment 4 Ryuan Choi 2012-07-23 15:53:40 PDT
(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.
Comment 5 Chris Dumez 2012-07-23 23:13:18 PDT
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.
Comment 6 Ryuan Choi 2012-07-24 00:58:44 PDT
Created attachment 153981 [details]
Patch
Comment 7 Ryuan Choi 2012-07-24 00:59:57 PDT
(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.
Comment 8 Chris Dumez 2012-07-24 01:22:08 PDT
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.
Comment 9 Ryuan Choi 2012-07-24 01:32:10 PDT
Created attachment 153986 [details]
Patch
Comment 10 Chris Dumez 2012-07-24 01:37:00 PDT
Comment on attachment 153986 [details]
Patch

LGTM.
Comment 11 Hajime Morrita 2012-07-25 00:53:03 PDT
Comment on attachment 153986 [details]
Patch

rs=me.
Comment 12 WebKit Review Bot 2012-07-25 02:04:17 PDT
Comment on attachment 153986 [details]
Patch

Clearing flags on attachment: 153986

Committed r123591: <http://trac.webkit.org/changeset/123591>
Comment 13 WebKit Review Bot 2012-07-25 02:04:23 PDT
All reviewed patches have been landed.  Closing bug.