RESOLVED FIXED 92001
[EFL][WK2] Update uri when the active URI is changed while loading.
https://bugs.webkit.org/show_bug.cgi?id=92001
Summary [EFL][WK2] Update uri when the active URI is changed while loading.
Ryuan Choi
Reported 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.
Attachments
Patch (8.45 KB, patch)
2012-07-23 08:13 PDT, Ryuan Choi
no flags
Patch (9.58 KB, patch)
2012-07-23 15:48 PDT, Ryuan Choi
no flags
Patch (9.77 KB, patch)
2012-07-24 00:58 PDT, Ryuan Choi
no flags
Patch (9.97 KB, patch)
2012-07-24 01:32 PDT, Ryuan Choi
no flags
Ryuan Choi
Comment 1 2012-07-23 08:13:09 PDT
Chris Dumez
Comment 2 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, ...
Ryuan Choi
Comment 3 2012-07-23 15:48:14 PDT
Ryuan Choi
Comment 4 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.
Chris Dumez
Comment 5 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.
Ryuan Choi
Comment 6 2012-07-24 00:58:44 PDT
Ryuan Choi
Comment 7 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.
Chris Dumez
Comment 8 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.
Ryuan Choi
Comment 9 2012-07-24 01:32:10 PDT
Chris Dumez
Comment 10 2012-07-24 01:37:00 PDT
Comment on attachment 153986 [details] Patch LGTM.
Hajime Morrita
Comment 11 2012-07-25 00:53:03 PDT
Comment on attachment 153986 [details] Patch rs=me.
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2012-07-25 02:04:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.