After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 683327 - Split ephy_snapshot_service_get_snapshot_async()
Split ephy_snapshot_service_get_snapshot_async()
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-09-04 13:02 UTC by Carlos Garcia Campos
Modified: 2012-09-06 08:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (39.76 KB, patch)
2012-09-04 13:02 UTC, Carlos Garcia Campos
committed Details | Review

Description Carlos Garcia Campos 2012-09-04 13:02:20 UTC
Created attachment 223421 [details] [review]
Patch

ephy_snapshot_service_get_snapshot_async() receives an option web view parameter, that it's only used in case the snapshot is not the in the thumbnails cache. We can split the method into ephy_snapshot_service_get_snapshot_async() to get a snapshot from a web view and ephy_snapshot_service_get_snapshot_for_url_async() to get a snapshot from the cache. The former uses the latter to try first if the web view URI is in the cache.
Comment 1 Claudio Saavedra 2012-09-04 17:35:35 UTC
Review of attachment 223421 [details] [review]:

So I'm principle I am in favor of the patch. Being frank, this does need a cleanup, so thanks a lot for working on it.

That being said I'd appreciate you could split it in smaller patches if possible so that I have an easier time reviewing it.
Comment 2 Carlos Garcia Campos 2012-09-04 18:24:39 UTC
My initial idea was to split it in several commits, but it's not that easy because get_snapshot() uses get_snpashot_for_url() and save_snapshot(). Maybe you can just apply the patch and review the code directly instead of the diff.
Comment 3 Claudio Saavedra 2012-09-06 05:23:04 UTC
Review of attachment 223421 [details] [review]:

OK, the patch looks good. I still think that there are some minor and not that minor style fixes that could have been split apart, but I leave it up to you to commit as is if you want.

::: lib/ephy-snapshot-service.c
@@ +275,2 @@
 #ifdef HAVE_WEBKIT2
+  if (webkit_web_view_get_estimated_load_progress (WEBKIT_WEB_VIEW (data->web_view)) == 1.0)

We need to fix this. As you can see in the #else, we should use the get_load_status()-equivalent instead (load_progress() doesn't always bring up good results. This is unrelated to the patch, I know.)

@@ +558,3 @@
+  g_simple_async_result_run_in_thread (result,
+                                       (GSimpleAsyncThreadFunc)save_snapshot_thread,
+  g_simple_async_result_set_op_res_gpointer (result,

This method is ignoring the cancellable parameter, right?
Comment 4 Carlos Garcia Campos 2012-09-06 07:42:56 UTC
(In reply to comment #3)
> Review of attachment 223421 [details] [review]:
> 
> OK, the patch looks good. I still think that there are some minor and not that
> minor style fixes that could have been split apart, but I leave it up to you to
> commit as is if you want.
> 
> ::: lib/ephy-snapshot-service.c
> @@ +275,2 @@
>  #ifdef HAVE_WEBKIT2
> +  if (webkit_web_view_get_estimated_load_progress (WEBKIT_WEB_VIEW
> (data->web_view)) == 1.0)
> 
> We need to fix this. As you can see in the #else, we should use the
> get_load_status()-equivalent instead (load_progress() doesn't always bring up
> good results. This is unrelated to the patch, I know.)

Yes, I noticed it, but it's indeed a different issue. There's no equivalent for load_status in WebKit2, but I have always wanted to add a is-loading property to WebKitWebView, that would make several things easier. 

> @@ +558,3 @@
> +  g_simple_async_result_run_in_thread (result,
> +                                      
> (GSimpleAsyncThreadFunc)save_snapshot_thread,
> +  g_simple_async_result_set_op_res_gpointer (result,
> 
> This method is ignoring the cancellable parameter, right?

No, cancellable is passed to run_in_thread, that checks whether it has been cancelled before calling save_snapshot_thread, and after it's called before completing the operation. The cancellable is also passed to save_snapshot_thread as a parameter in case operations in the thread can be cancelled, but it's not our case.