GNOME Bugzilla – Bug 683327
Split ephy_snapshot_service_get_snapshot_async()
Last modified: 2012-09-06 08:26:35 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.
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.
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.
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?
(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.