GNOME Bugzilla – Bug 641739
Hide EphySession asynchronicity implementation details (command queue)
Last modified: 2013-01-08 17:03:46 UTC
EphySession provides async calls by the mean of a command queue. The current API exposes the following call: void ephy_session_queue_command (EphySession *session, EphySessionCommand op, const char *arg, char **args, guint32 user_time, gboolean priority); There is currently very few command types handled (open uris, restore...). This approach exposes the inner working of the async support. Instead, it might be nicer to isolate that by convenience methods. This would allow us to change the implementation (using thread, continuations, signals, camembert for ex.) without worrying the client code. And makes it easier to human-parse. For example, we could expose: void ephy_session_open_uris_async (EphySession *session, char **uris, const char *options, guint32 user_time); I am still not sure if we should expose their synchronous counterparts. They are currently private, anyway.
Created attachment 221041 [details] [review] Remove EPHY_SESSION_CMD_LOAD_SESSION This first patch removes EPHY_SESSION_CMD_LOAD_SESSION command
Created attachment 221042 [details] [review] Remove EPHY_SESSION_CMD_RESUME_SESSION
Created attachment 221043 [details] [review] Remove EPHY_SESSION_CMD_OPEN_BOOKMARKS_EDITOR
Created attachment 221045 [details] [review] Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW
Created attachment 221046 [details] [review] Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW_RESTORE
Created attachment 221049 [details] [review] Remove EPHY_SESSION_CMD_OPEN_URIS This is the last one, all commands code has been removed in this patch too.
Created attachment 221102 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Updated to open uris in an idle.
Created attachment 221130 [details] [review] Update: Remove EPHY_SESSION_CMD_RESUME_SESSION I forgot a couple of returns in ephy_session_resume()
Created attachment 223276 [details] [review] Update: Remove EPHY_SESSION_CMD_RESUME_SESSION Updated to apply on current git master, tool windows are not tracked by the session anymore.
Created attachment 223278 [details] [review] Update: Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW Updated to current git master, tool windows are no longer tracked by the session.
Created attachment 223279 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Updated to apply on current git master
Created attachment 224844 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Updated to handle EPHY_NEW_TAB_PRESENT_WINDOW added to git master
Created attachment 226026 [details] [review] Updated: Remove EPHY_SESSION_CMD_RESUME_SESSION Updated for the changes in bug #641734 (ephy_shell_has_windows() has been removed)
Created attachment 226027 [details] [review] Update: Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW Updated for the changes in bug #641734 (ephy_shell_has_windows() has been removed)
Created attachment 229888 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Patch updated after a60eb7cad58e17985e8a7908d5e90bab5fff6186 (ephy_shell_get_active_window() has been removed)
Comment on attachment 221043 [details] [review] Remove EPHY_SESSION_CMD_OPEN_BOOKMARKS_EDITOR This doesn't exist in current git master
Created attachment 231981 [details] [review] Update: Remove EPHY_SESSION_CMD_LOAD_SESSION
Created attachment 231982 [details] [review] Update: Remove EPHY_SESSION_CMD_RESUME_SESSION
Created attachment 231983 [details] [review] Update: Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW
Created attachment 231984 [details] [review] Update: Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW_RESTORE
Created attachment 231985 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS
OK, I have a couple of issues with this bug. Perhaps not so much in the changes themselves, but in how they are justified. - The first comment does not make sense to me. Exposing async APIs (or sync) instead of a command based API does not simplify things, neither does it "hide" anything (in fact I'd say it does exactly the opposite). Carlos didn't say whether he agrees with this or not, so I'm not sure of his motives to do these patches. - From what I can tell the patches essentially just move some complexity to the API surface (with more methods exposed that explicitly use the async mechanisms in glib). I'm not sure I understand the value in this, but perhaps it makes sense when coupled with things in other bugs. Perhaps it makes sense to do things like this instead of the current way in which this is async (doing stuff in idles), but then again that seems orthogonal to changing how the API looks like. So in short: - What is the overall motivation to do this? - Is this strongly coupled with other refactorings in other bugs? - Is there a strong motivation to change the API or do we just want to get rid of the idle+queue stuff? I think it's important to write those things down in the bug/patches, so let's start doing this.
(In reply to comment #22) > OK, I have a couple of issues with this bug. Perhaps not so much in the changes > themselves, but in how they are justified. > > - The first comment does not make sense to me. Exposing async APIs (or sync) > instead of a command based API does not simplify things, neither does it "hide" > anything (in fact I'd say it does exactly the opposite). Carlos didn't say > whether he agrees with this or not, so I'm not sure of his motives to do these > patches. I agree that having async methods instead of an API to queue commands makes it use a lot simpler. I agree it doesn't hide anything though, maybe I should have opened a new bug instead of reusing this one. > - From what I can tell the patches essentially just move some complexity to the > API surface (with more methods exposed that explicitly use the async mechanisms > in glib). I'm not sure I understand the value in this, but perhaps it makes > sense when coupled with things in other bugs. Perhaps it makes sense to do > things like this instead of the current way in which this is async (doing stuff > in idles), but then again that seems orthogonal to changing how the API looks > like. If we have proper async methods to do the things with proper error reporting, etc. we simply don't need a common queue command API, why do we need a queue if we have a main loop to schedule tasks? Some of the commands don't really need to be queued like MAYBE_OPEN_WINDOW. I'm not sure what API surface means, but the only patch that moves the code to a different place is the open uris one, that moves the implementation to ephy-shell. The previous code converted all the open options into a string to pass them to the command queue, and then the string is parsed into open options again when the command is processed. Since the options are in the shell and open the uris is part of the application startup, why doing that job in the session? > So in short: > > - What is the overall motivation to do this? Simplify the code. > - Is this strongly coupled with other refactorings in other bugs? Yes, previous bugs made the session load async, so we don't really need to queue a method that is already async. > - Is there a strong motivation to change the API or do we just want to get rid > of the idle+queue stuff? Both, get rid of the queue and provide an async API. > I think it's important to write those things down in the bug/patches, so let's > start doing this. Fair enough
Review of attachment 231981 [details] [review]: OK, this one is simple enough.
Review of attachment 231982 [details] [review]: ::: src/ephy-session.c @@ +1302,3 @@ + + ephy_session_load (session, SESSION_STATE, user_time, cancellable, + session_resumed_cb, result); Here you are copying the old code for this method, which is confusing and relies on the early return and the side-effects of the MAYBE_OPEN_WINDOW method to work correctly. Please use the updated code which you are removing in the same patch.
Review of attachment 231983 [details] [review]: ::: src/ephy-session.c @@ +292,3 @@ + return; + } + Nitpick, but I think this is pretty ugly. I'd just do a == 0 check and put the code inside the if.
Review of attachment 231984 [details] [review]: Heh.
Created attachment 232731 [details] [review] Update: Remove EPHY_SESSION_CMD_RESUME_SESSION Updated patch according to review comments
Created attachment 232732 [details] [review] Update: Remove EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW Updated for changes in previous patch.
Review of attachment 232731 [details] [review]: ::: src/ephy-session.c @@ +1298,3 @@ + ephy_session_load (session, SESSION_STATE, user_time, cancellable, + session_resumed_cb, result); + return; This return is wrong no? Looks good otherwise.
(In reply to comment #26) > Review of attachment 231983 [details] [review]: > > ::: src/ephy-session.c > @@ +292,3 @@ > + return; > + } > + > > Nitpick, but I think this is pretty ugly. Indeed it is :-) > I'd just do a == 0 check and put the > code inside the if.
(In reply to comment #30) > Review of attachment 232731 [details] [review]: > > ::: src/ephy-session.c > @@ +1298,3 @@ > + ephy_session_load (session, SESSION_STATE, user_time, cancellable, > + session_resumed_cb, result); > + return; > > This return is wrong no? Looks good otherwise. No it's not, we don't want to complete the operation when ephjy_session_load is called, it will be completed when the session is actually loaded in session_resumed_cb
Review of attachment 231985 [details] [review]: I think to keep in the spirit of the previous patches this should: - Export ephy_session_open_uris, use it from shell (change the test, and it still should pass) - Then make a different patch removing all the now useless crap in EphySession After that we can see if it makes sense to shortcircuit the shell->session->shell roundtrip when opening URIs or not, but refactorings should be mindless, and adding #if 0 to the test is icky.
Created attachment 232958 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Updated according to review comments
Created attachment 232959 [details] [review] Remove session command API
Created attachment 232965 [details] [review] Update: Remove EPHY_SESSION_CMD_OPEN_URIS Fix the unit tests
Created attachment 232966 [details] [review] Update: Remove session command API Update for the changes in previous commit
Review of attachment 232965 [details] [review]: OK, looks good to me. ::: src/ephy-session.h @@ +29,2 @@ #include "ephy-window.h" +#include "ephy-shell.h" Same comment about the header order.
Review of attachment 232732 [details] [review]: ::: src/ephy-session.c @@ +291,3 @@ + { + return; + } Well since we both agree this looks ugly I repeat my suggestion of just putting the code inside the if? Early return for one line methods really make no sense. OK to commit with that change.
Review of attachment 232966 [details] [review]: Cool!
(In reply to comment #39) > Review of attachment 232732 [details] [review]: > > ::: src/ephy-session.c > @@ +291,3 @@ > + { > + return; > + } > > Well since we both agree this looks ugly I repeat my suggestion of just putting > the code inside the if? Early return for one line methods really make no sense. > OK to commit with that change. Sure, I fixed the conflicts but forgot to check review comments, sorry.
Pushed all patches to git master, thanks for reviewing.