GNOME Bugzilla – Bug 641734
Split EphySession
Last modified: 2012-10-08 13:31:02 UTC
EphySession currently has several unrelated responsibilities - session-mananger client - command queue handling - browsing session de/serialization - browser session resuming (logic and ui) - shutdown confirmation ui In order to make the code easier to follow and better segmented, and avoid state leaks and the associated potential side-effects, those concerns should be taken care of by separate components.
*** Bug 641737 has been marked as a duplicate of this bug. ***
Created attachment 220975 [details] [review] Move windows handling code from EphySession to EphyShell
(In reply to comment #2) > Created an attachment (id=220975) [details] [review] > Move windows handling code from EphySession to EphyShell The patch does not explain why it does this.
Created attachment 221100 [details] [review] Updated patch Explain why in commit message.
Created attachment 223271 [details] [review] Patch updated to current git master Tool windows are not tracked anymore by the session.
Created attachment 225249 [details] [review] Updated to current git master Updated for the ephy_session_close_all_windows change
Review of attachment 225249 [details] [review]: I like where this goes in general, just some comments. The commit message mentions tool windows, that's obsolete now. ::: src/ephy-session.c @@ +67,3 @@ #define SESSION_STATE "type:session_state" +G_DEFINE_TYPE (EphySession, ephy_session, G_TYPE_OBJECT) Cool, no more extensions in tree! ::: src/ephy-shell.h @@ +184,3 @@ +guint ephy_shell_get_n_windows (EphyShell *shell); + +EphyWindow *ephy_shell_get_active_window (EphyShell *shell); I don't think having all these methods makes sense. get_active_window and get_windows should be enough. has_windows just means whether get_windows is not NULL, and get_n_windows is trivial to implement on top of it? At the very least, if we really really want them, do them in future patches.
Uh, EphyLockdown is still an extension, so we need to fix that too. Filed in bug #685631.
(In reply to comment #7) > Review of attachment 225249 [details] [review]: I've just noticed that I was not in the CC and I had missed the review :-P Thanks for looking at it. > I like where this goes in general, just some comments. The commit message > mentions tool windows, that's obsolete now. Oh, right. > ::: src/ephy-session.c > @@ +67,3 @@ > #define SESSION_STATE "type:session_state" > > +G_DEFINE_TYPE (EphySession, ephy_session, G_TYPE_OBJECT) > > Cool, no more extensions in tree! Yes :-) > ::: src/ephy-shell.h > @@ +184,3 @@ > +guint ephy_shell_get_n_windows (EphyShell *shell); > + > +EphyWindow *ephy_shell_get_active_window (EphyShell *shell); > > I don't think having all these methods makes sense. get_active_window and > get_windows should be enough. has_windows just means whether get_windows is not > NULL, and get_n_windows is trivial to implement on top of it? At the very > least, if we really really want them, do them in future patches. These are convenient methods to be able to know whether there are windows and how many without having to copy the list, I agree that has_windows can be get_n_windows() > 0, so I'll remove has_windows()
Created attachment 226021 [details] [review] Updated patch Rebased to current git master and addressed review comments.
Review of attachment 226021 [details] [review]: OK.
Comment on attachment 226021 [details] [review] Updated patch Thanks!