GNOME Bugzilla – Bug 479919
EphyTab should connect to abstract properties instead of connecting to signals from the backend
Last modified: 2007-10-27 15:44:45 UTC
As stated in bug #461652, EphyTab should not connect to GtkMozEmbed signals directly, but we should use notify on properties, to make Epiphany integrate cleaner with new backends. Chpe's comment about it: "I think what we should do here is to instead of rely on those signals, we should make the embed widget use object properties. E.g. instead of catching the progress signals in EphyTab, we should move that code to the EphyEmbed implementations and have it only update the "load-status" and "load-progress" properties. Same for title, document type, etc. (Remember that we're no longer tied to gtkmozembed once the xulrunner backend becomes the default (and the mozilla [==gecko 1.8] one goes away! So we can and should do away with that ugly stuff.)".
Some random comments about the signals before I forget it again: - "size-to": used in EphyWindow to eventually call some (hackish) methods in EphyTab. chpe says we need to refactor ephy to not need EphyTab in order to get rid of that. - "destroy-browser": used in EphyWindow, the callback is simply a g_return_if_reached () though. - "open-uri": used in EphyTab to workaround a bug in Mozilla. - "visibility": simply used to control the visibility of EphyTab. I think this can be moved now to a property in EphyEmbed that the backends will update. chpe? - "link-message": this is not listed in the header but comes from GtkMozEmbed too. Seems can be moved now too? - "title": used in EphyTab to update the title, can be moved to a property. - "net-stop": used in EphySession to save the session in case of a crash from what I see in the code. Well, that's it I think. Corrections and comments welcome, I think we can start with visibility and title at least.
Looks good to me :) - destroy-browser is unused, it should never be emitted (the g_return_if_reached() is just to alert us if it was emitted :) - open-uri: the workaround is only needed to set some properties that will move too, so this signal can be removed - visibility: the purpose of this signal isn't clear to me at all. let's just move it for now and worry about it / remove it later - link-message: move too (use to set the status message property) - title: move to property - net-stop: if session uses this, it should just listen to the load-status property instead
Created attachment 97289 [details] [review] first non-working try Attached is a first (non compiling) attempt to move everything. Some comments: - I did not delete or move anything from EphyTab yet. - EphyWindow still connects to EphyTab instead of EphyEmbed. - I cannot make it compile, I think because of some auto{conf,make,gen} dep voodoo I can't figure out...maybe someone with more magic powers than me can solve this. - Maybe I moved some parts which are not to be moved (Popup parts for example, I don't know). I'd be glad if someone could help me on this, and/or tell me if I'm going in the right direction, maybe also do some part of this job, as it is really awful :P
Can you show me the exact compile/link errors, so I can help? :)
These are the errors...some of them are for missing defines or enums, and they should be no problem, but the other are probably due to some bad linking with GnomeVFS and EphyEmbed somewhere. Thanks! :) mozilla-embed.cpp: In function 'void mozilla_embed_class_init(MozillaEmbedClass*)': mozilla-embed.cpp:513: error: 'EPHY_TYPE_TAB_NAVIGATION_FLAGS' was not declared in this scope mozilla-embed.cpp:580: error: 'ZOOM_MINIMAL' was not declared in this scope mozilla-embed.cpp:581: error: 'ZOOM_MAXIMAL' was not declared in this scope mozilla-embed.cpp: In function 'char* get_title_from_address(const char*)': mozilla-embed.cpp:624: error: invalid conversion from 'int' to 'GnomeVFSURIHideOptions' mozilla-embed.cpp:624: error: initializing argument 2 of 'gchar* gnome_vfs_uri_to_string(const GnomeVFSURI*, GnomeVFSURIHideOptions)' mozilla-embed.cpp: In function 'void mozilla_embed_update_navigation_flags(MozillaEmbed*)': mozilla-embed.cpp:1686: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1690: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1695: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1700: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp: In function 'void mozilla_embed_net_state_all_cb(GtkMozEmbed*, const char*, gint, guint, MozillaEmbed*)': mozilla-embed.cpp:2002: error: invalid conversion from 'gint' to 'EphyEmbedNetState' mozilla-embed.cpp:2002: error: initializing argument 3 of 'void update_net_state_message(MozillaEmbed*, const char*, EphyEmbedNetState)' mozilla-embed.cpp:2046: error: invalid conversion from 'gint' to 'EphyEmbedNetState' mozilla-embed.cpp:2046: error: initializing argument 2 of 'void build_progress_from_requests(MozillaEmbed*, EphyEmbedNetState)' mozilla-embed.cpp: In function 'gboolean mozilla_embed_get_popups_allowed(MozillaEmbed*)': mozilla-embed.cpp:2530: error: 'CONF_SECURITY_ALLOW_POPUPS' was not declared in this scope
Created attachment 97332 [details] [review] Removes some warnings from previous patch So to not feel useless and lazy, I gave this patch an update, I corrected some of the missing includes and added some castings (like EphyEmbedNetState in 2048). Now the warnings printed are: mozilla-embed.cpp: In function 'char* get_title_from_address(const char*)': mozilla-embed.cpp:627: error: invalid conversion from 'int' to 'GnomeVFSURIHideOptions' mozilla-embed.cpp:627: error: initializing argument 2 of 'gchar* gnome_vfs_uri_to_string(const GnomeVFSURI*, GnomeVFSURIHideOptions)' I tried with a small .c to call this function with the same arguments, it worked. Can this be a C++ thing? Does '|' mean something different in C++ than in C (I understand it doesn't)? mozilla-embed.cpp: In function 'void mozilla_embed_update_navigation_flags(MozillaEmbed*)': mozilla-embed.cpp:1689: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1693: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1698: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' mozilla-embed.cpp:1703: error: invalid conversion from 'int' to 'EphyEmbedNavigationFlags' No idea :) make[4]: *** [libephymozillaembed_la-mozilla-embed.lo] Error 1 make[3]: *** [all-recursive] Error 1 make[2]: *** [all] Error 2 make[1]: *** [all-recursive] Error 1 make: *** [all] Error 2
Cast like this: GnomeVFSURIHideOptions(FLAG_1 | FLAG_2 | FLAG_3). And for this: + EphyEmbedNavigationFlags flags = 0; + + if (ephy_embed_can_go_up (EPHY_EMBED (embed))) + { + flags |= EPHY_EMBED_NAV_UP; + } you can either use flags = EphyEmbedNavigationFlags(flags | NEW_FLAG) or guint flags = 0; flags |= ... and only cast in the final assignment.
Created attachment 97377 [details] [review] No more warnings, but segfaults Ok, no more warnings but now it segfaults! Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1243764528 (LWP 10924)] mozilla_embed_init (embed=0x86aa8b8) at mozilla-embed.cpp:781 781 priv->is_loading = FALSE; Current language: auto; currently c++
Instead of + MozillaEmbedPrivate *priv = embed->priv; + embed->priv = MOZILLA_EMBED_GET_PRIVATE (embed); you want + MozillaEmbedPrivate *priv; + + priv = embed->priv = ..._GET_PRIVATE(...)
That works!. What would be the next step?
Next step would be to remove the code from ephy-tab.c, and make ephy-window.c (and all others too) connect to the properties on the embed instead of on the tab. (Do disable all extensions for now, updating them will be another step.)
Created attachment 97404 [details] [review] removed functions from EphyTab I made some more progress, removing copied functions from ephy-tab.c. I did not remove the properties yet, and it does not compile, but I'm unsure what we shall do with the ephy_tab_get_* functions. Can't we just use g_object_get () to retrieve that data and avoid to copy those functions?
Created attachment 97405 [details] [review] removed functions from ephy-tab.c Oops, picked up wrong patch sorry :)
(In reply to comment #12) > I made some more progress, removing copied functions from ephy-tab.c. > I did not remove the properties yet, and it does not compile, but I'm unsure > what we shall do with the ephy_tab_get_* functions. Can't we just use > g_object_get () to retrieve that data and avoid to copy those functions? Using g_object_get for the string properties has the disadvantage that it makes a string copy, while we don't need one. I think that for those ephy_tab_get_* functions that return |const char*|, we should just move those functions as ephy_embed_get_* so we save that copying.
So, this looks great, but I don't really work to like with mega-patches whenever possible. Seeing nobody seems to know what the visibility signal is good for, I just went ahead and removed its usage from src/ (including the property in EphyTab). I don't see any difference in the behavior of the browser :) Can we commit that to make the patch slightly smaller and focused on the hard bits?
Created attachment 97515 [details] [review] 0001-Completely-remove-usage-of-visibility-signal-from-src Subject: [PATCH] Completely remove usage of visibility signal from src/ --- src/ephy-tab.c | 54 ----------------------------------------------------- src/ephy-tab.h | 2 - src/ephy-window.c | 33 -------------------------------- src/epiphany.defs | 6 ----- 4 files changed, 0 insertions(+), 95 deletions(-)
Created attachment 97519 [details] [review] Cosimo's patch updated to trunk Ok, this is the mega-patch updated to apply to trunk.
Created attachment 97532 [details] [review] Move security-level to EphyEmbed This puts the property in the interface itself, implementations need to set it themselves or override it.
Created attachment 97533 [details] [review] Tabify-mozilla-embed.cpp-and-make-it-use-G_DEFINE Tabify mozilla-embed.cpp (it was a mess) and make it use G_DEFINE_*. Also, remove one unneeded call to ephy_tab_get_embed in EphyWindow.
Created attachment 97549 [details] [review] Move-document-type-property-from-EphyTab-to-EphyEmbed.patch Subject: [PATCH] Move document-type property from EphyTab to EphyEmbed. --- embed/ephy-embed.c | 23 ++++++++++++++++++ embed/ephy-embed.h | 32 ++++++++++++++----------- embed/mozilla/mozilla-embed.cpp | 47 ++++++++++++++++++++++++++++++++++---- src/ephy-tab.c | 48 --------------------------------------- src/ephy-tab.h | 2 - src/ephy-window.c | 31 +++++++++++++----------- src/epiphany.defs | 6 ----- 7 files changed, 100 insertions(+), 89 deletions(-)
Created attachment 97552 [details] [review] Move-zoom-from-EphyTab-to-EphyEmbed.patch Subject: [PATCH] Move zoom from EphyTab to EphyEmbed. --- embed/ephy-embed.c | 9 +++ embed/mozilla/mozilla-embed.cpp | 112 ++++++++++++++++++++++++++++++++++++- src/ephy-tab.c | 119 +-------------------------------------- src/ephy-window.c | 26 ++++---- src/epiphany.defs | 8 --- 5 files changed, 134 insertions(+), 140 deletions(-)
Created attachment 97570 [details] [review] ove-load-progress-from-EphyTab-to-EphyEmbed.patch Subject: [PATCH] Move load-progress from EphyTab to EphyEmbed --- embed/ephy-embed.c | 38 ++++++++++++++++++++++++ embed/ephy-embed.h | 8 ++++- embed/mozilla/mozilla-embed.cpp | 31 ++++++++++++++++++++ src/ephy-tab.c | 61 ++------------------------------------- src/ephy-window.c | 20 ++++++------ src/epiphany.defs | 6 ---- 6 files changed, 89 insertions(+), 75 deletions(-) -- I've added ephy_embed_set_load_percent and kept the property in EphyEmbed "read-only" as EphyTab did, not entirely sure if it makes sense.
Created attachment 97571 [details] [review] Use-correct-property-enum.patch Oops :)
Created attachment 97576 [details] [review] Move-load-status-from-EphyTab-to-EphyEmbed.patch Subject: [PATCH] Move load-status from EphyTab to EphyEmbed. --- embed/ephy-embed.c | 35 ++++++++++++++++++- embed/ephy-embed.h | 10 ++++-- embed/mozilla/mozilla-embed.cpp | 37 ++++++++++++++++++++ src/ephy-notebook.c | 10 +++--- src/ephy-session.c | 2 +- src/ephy-tab.c | 73 ++++++++++---------------------------- src/ephy-window.c | 22 ++++++------ src/epiphany.defs | 6 --- 8 files changed, 114 insertions(+), 81 deletions(-)
Created attachment 97583 [details] [review] [1/1] Move navigation property from EphyTab to EphyEmbed. embed/ephy-embed.c | 30 ++++++++++++++++- embed/ephy-embed.h | 18 +++++++++- embed/mozilla/mozilla-embed.cpp | 45 ++++++++++++++++++++++++++ src/ephy-tab.c | 67 ++------------------------------------- src/ephy-tab.h | 9 ----- src/ephy-window.c | 30 +++++++---------- src/epiphany.defs | 38 +++++++++------------- 7 files changed, 121 insertions(+), 116 deletions(-)
Created attachment 97586 [details] [review] Update WebKit backend to new changes and implements load progress. Attached patch adapts WebKit backend to Xan's today changes (does not segfault anymore) and implements load progress properly for WebKit. To see it work in all its coolness, we still have to comment the ephy_embed_set_load_percent in EphyTab, which will disappear when we'll move that code to Mozilla backend.
Created attachment 97655 [details] [review] Move link-message property to EphyEmbed and make MozillaEmbed use it. Attached patch moves link-message property from EphyTab to EphyEmbed. I also addes ephy-embed-utils.[ch] to contain the functions which are backend-independent (right now there's only ex-EphyTab function that parses email address to show them pretty in the statusbar). Also, i splitted "message" property, and added "link-message" to move the link-message parts without moving too much things (completely moving message also involves some other properties) and updated (and added a FIXME comment) ephy-window.c according, though it may not be a bad idea to separate "link-message" and "status-message", as it would IMHO simplify code a bit.
Created attachment 97656 [details] [review] oops, dropped a g_free () Dropped a g_free () in previous patch, sorry :)
Created attachment 97658 [details] [review] adds missing get_property and set_property for MozillaEmbed Again, i missed Mozilla get_property and set_property. WebKit bit to follow soon.
Created attachment 97663 [details] [review] WebKit implementation of link-message. Attached patch adds link message support using new EphyEmbed API.
Created attachment 97805 [details] [review] [1/10] Move address and typed-address from EphyTab to EphyEmbed ephy-tab.c is totally broken now, will fix in next patches. --- embed/ephy-embed.c | 75 +++++++++++++++++++- embed/ephy-embed.h | 22 ++++++ embed/mozilla/mozilla-embed.cpp | 110 ++++++++++++++++++++++++++-- src/ephy-lockdown.c | 4 +- src/ephy-shell.c | 4 +- src/ephy-tab.c | 154 +-------------------------------------- src/ephy-tab.h | 19 ----- src/ephy-toolbar.c | 4 +- src/epiphany.defs | 28 ++++---- 9 files changed, 221 insertions(+), 199 deletions(-)
Created attachment 97806 [details] [review] [2/10] Move title from EphyTab to EphyEmbed. EphyTab still borken. --- embed/ephy-embed.c | 159 +++++++---------- embed/ephy-embed.h | 12 +- embed/mozilla/mozilla-embed.cpp | 346 +++++++++++++++++++++++++++++++++++-- src/ephy-tab.c | 358 +-------------------------------------- 4 files changed, 400 insertions(+), 475 deletions(-)
Created attachment 97807 [details] [review] [3/10] Change ephy_embed_get_title to return const char*. Also replace all ephy_tab_get_title by ephy_embed_get_title. --- embed/ephy-embed.c | 2 +- embed/ephy-embed.h | 4 ++-- embed/mozilla/mozilla-embed.cpp | 4 ++-- src/ephy-location-action.c | 2 +- src/ephy-notebook.c | 2 +- src/ephy-session.c | 2 +- src/ephy-tab.h | 2 -- src/ephy-tabs-menu.c | 2 +- src/ephy-window.c | 2 +- src/epiphany.defs | 8 +------- src/window-commands.c | 2 +- 11 files changed, 12 insertions(+), 20 deletions(-)
Created attachment 97808 [details] [review] [4/10] Move status-message and logic of link-message to EphyEmbed. Create a property for link-message in EphyEmbed too. --- embed/ephy-embed.c | 37 ++++- embed/ephy-embed.h | 8 +- embed/mozilla/mozilla-embed.cpp | 322 ++++++++++++++++++++++++++++++++++- src/ephy-tab.c | 362 --------------------------------------- 4 files changed, 357 insertions(+), 372 deletions(-)
Created attachment 97809 [details] [review] [5/10] Move icon and icon-address to EphyEmbed. There's a snippet that needs to be moved elsewhere I think: eb = ephy_shell_get_bookmarks (ephy_shell); ephy_bookmarks_set_icon (eb, priv->address, priv->icon_address); It's done in mozilla_embed_set_icon_address. --- embed/ephy-embed.c | 44 +++++++++++ embed/ephy-embed.h | 7 ++ embed/mozilla/mozilla-embed.cpp | 145 +++++++++++++++++++++++++++++++--- src/ephy-tab.c | 164 --------------------------------------- 4 files changed, 184 insertions(+), 176 deletions(-)
Created attachment 97810 [details] [review] [6/10] Move title/address update on open-uri signal to EphyEmbed. Also refactor some common code in ephy-embed-utils.c --- embed/Makefile.am | 2 + embed/ephy-embed-utils.c | 119 +++++++++++++++++++++++++++++++++++++++ embed/ephy-embed-utils.h | 37 ++++++++++++ embed/ephy-embed.c | 21 +++++++ embed/ephy-embed.h | 8 ++- embed/mozilla/mozilla-embed.cpp | 44 ++++++++++++++ src/ephy-tab.c | 64 --------------------- 7 files changed, 230 insertions(+), 65 deletions(-)
Created attachment 97811 [details] [review] [7/10] Move file_monitor_cancel on dispose from EphyTab to EphyEmbed. embed/mozilla/mozilla-embed.cpp | 11 +++++++++++ src/ephy-tab.c | 2 -- 2 files changed, 11 insertions(+), 2 deletions(-)
Created attachment 97812 [details] [review] [8/10] Port src/ to the new properties in EphyEmbed. src/ephy-location-action.c | 2 +- src/ephy-notebook.c | 4 +- src/ephy-session.c | 2 +- src/ephy-tab.h | 19 ----------- src/ephy-tabs-menu.c | 3 +- src/ephy-window.c | 73 ++++++++++++++++++++++--------------------- src/epiphany.defs | 63 +------------------------------------- src/prefs-dialog.c | 2 +- src/window-commands.c | 10 +++--- 9 files changed, 50 insertions(+), 128 deletions(-)
Created attachment 97813 [details] [review] [9/10] Fix the remaining breakage. Implement get_icon and get_icon_address. Fix status-message canonical name. Properly assign private pointer in MozillaEmbed (...) --- embed/ephy-embed.c | 4 +++- embed/mozilla/mozilla-embed.cpp | 27 ++++++++++++++++++++++++--- src/ephy-window.c | 2 +- 3 files changed, 28 insertions(+), 5 deletions(-)
Created attachment 97814 [details] [review] [10/10] Fix more issues left with the refactoring. Fix double free in link_message property. Fix title by connecting to embed and not to tab in EphyWindow. Fix progress by passing the correct parameter to update_embed_from_net_state. Also use the link_message parsing function in utils. --- embed/ephy-embed-utils.c | 2 +- embed/mozilla/mozilla-embed.cpp | 69 ++++++++++---------------------------- src/ephy-window.c | 2 +- 3 files changed, 20 insertions(+), 53 deletions(-)
Patch 1/10: + MozillaEmbedPrivate *priv = embed->priv; + priv = MOZILLA_EMBED_GET_PRIVATE (embed); + priv->browser = new EphyBrowser (); This is wrong. You want MEP *priv; priv = embed->priv = ME_GET_PRIVATE(...) I suppose one of the next patches fixes this :) Patch 2/10: ok Patch 3/10: ok Patch 4/10: ok Patch 5/10: ok Patch 6/10: embed/ephy-embed-utils.c needs to include config.h. Patch 7/10: ok Patch 8/10: ok Patch 9/10: ok Patch 10/10: ok Let's also bump EPIPHANY_API_VERSION to 2.21.1 in configure.ac so as to not load non-ported extensions.
(In reply to comment #41) > Patch 1 [edit]/10: > > + MozillaEmbedPrivate *priv = embed->priv; > + priv = MOZILLA_EMBED_GET_PRIVATE (embed); > + priv->browser = new EphyBrowser (); > > This is wrong. You want > > MEP *priv; > priv = embed->priv = ME_GET_PRIVATE(...) > > I suppose one of the next patches fixes this :) Yep :P > > > Patch 2 [edit]/10: ok > > Patch 3 [edit]/10: ok > > Patch 4 [edit]/10: ok > > Patch 5 [edit]/10: ok > > Patch 6 [edit]/10: > > embed/ephy-embed-utils.c needs to include config.h. > > Patch 7 [edit]/10: ok > > Patch 8 [edit]/10: ok > > Patch 9 [edit]/10: ok > > Patch 10 [edit]/10: ok > > Let's also bump EPIPHANY_API_VERSION to 2.21.1 in configure.ac so as to not > load non-ported extensions. > Ok, I'll commit that. Thanks for the review!
Created attachment 97950 [details] [review] [1/1] Move popups-allowed and hidden-popup-count from EphyTab to EphyEmbed. Popup management is non-working now, see bug #490672 --- embed/ephy-embed.c | 15 ++ embed/mozilla/mozilla-embed.cpp | 351 ++++++++++++++++++++++++++++++++- src/ephy-tab.c | 414 --------------------------------------- src/ephy-window.c | 29 ++-- 4 files changed, 375 insertions(+), 434 deletions(-)
+ case PROP_HIDDEN_POPUP_COUNT: + // g_value_set_int (value, popup_blocker_n_hidden (embed)); + case PROP_POPUPS_ALLOWED: + // g_value_set_boolean (value, mozilla_embed_get_popups_allowed (embed)); Leave the commented code in, but add a g_value_set_int/boolean setting the value to 0/FALSE.
I think we can close this now. The action continues in bug #490832.