Created attachment 92668 [details] [review]
Some bits of the first try

I have some better stuff now that upstream bugs are fixed, but I prefer to have bug #461689 fixed first.

You can commit the can_go stuff when you want, for the other things I'd rather commit something working. Thanks!

Created attachment 92679 [details] [review]
First thing to commit

This is what gets can_whatever working, without the ge_location thing it is useless anyway, but again the back button doesn't work so it's useless anyway.

Hmm thinking again, I'll commit the can_ stuff only anyway. I'll leave the signals and that out. And I'll fix those two compilation warnings (impl_has_modified_forms and impl_manager_can_do_command).

Thinking yet again, I'll just commit them as #if 0 so we save some typying to do tests.

Created attachment 92680 [details] [review]
To be committed now.

Looks like this in the end

Committed last patch.

Depending on http://bugs.webkit.org/show_bug.cgi?id=14812 to implement tab titles

Depending on http://bugs.webkit.org/show_bug.cgi?id=14811 to make back/forward work.

Created attachment 95745 [details] [review]
makes title and location update work

Cyril has solved webkit bug 14812, and made this patch to get it working into Epiphany. I just added a "title" signal so that ephy-tab actually updates the title the right time.

Created attachment 95854 [details] [review]
mega patch

This patch does a lot of things:

- includes the previous title and location updates
- includes support for many previously unsupported signals, this makes many things work more properly than now.
- adds support for EphyEmbed flags, so that now we have progress bar appearing and disappearing at the right time and window title and status bar updated when connecting/transfering to a site.
- adds workaround ported from GdkLauncher to parse input urls correctly (fixes bug that images were not shown).
- formats every file in embed/webkit to 8-tabs spaces.
- maybe something more i forgot.

Is it Xan that approves patches for Webkit embed?

Cool!. Let's wait for Xan's opinion.

Ok, I'm back now.

First of all, thanks for the patch :)

Second, in the future I'd appreciate if you separated actual code changes from (re)formatting stuff, it's much easier to review that way. About that, at this point of my life I kind of think that using tabs for indentation makes no sense, but I guess you think being consistent with the rest of epiphany is more important. Being a separate module inside ephy I don't think it's that relevant, but let's do it anyway... And btw, you seem to be introducing lots of indentation glitches?

-#define WEBKIT_EMBED(o)		(G_TYPE_CHECK_INSTANCE_CAST ((o), WEBKIT_TYPE_EMBED, WebKitEmbed))
+#define WEBKIT_EMBED(o)			(G_TYPE_CHECK_INSTANCE_CAST ((o), WEBKIT_TYPE_EMBED, WebKitEmbed))

Now the gory details:

+	webkit_embed_parent_class = (GObjectClass *) g_type_class_peek_parent (klass);

G_DEFINE_* already does this, no need to do it.

+	EphyEmbedNetState estate = EPHY_EMBED_STATE_UNKNOWN;

Isn't it spelled state?

+	g_signal_new ("title",

Why not keep the signal ids and emit using them? It's slightly faster.

+impl_load (EphyEmbed *embed,
+	   const char *url,
(...)
+	{
+	  GString *parsed_url = g_string_new ("http://");
+	  g_string_append (parsed_url, url);
+	  url = g_string_free (parsed_url, FALSE);
+	}

So you are never freeing the result of the append now? And you are assigning it to a const parameter, I wonder what happens if you try to free it. I'd rather rewrite this to use a new variable that you always dispose at the end.

+	wembed->priv->loading_uri = g_strdup (url);

This is never freed anywhere? (And you'll overwrite the previous contents). Free it before assigning (g_free is NULL safe) and free in once more in finalize.

+	gchar *title = webkit_gtk_frame_get_title (frame);
+	return g_strdup (title);

+	gchar *location = webkit_gtk_frame_get_location (frame);
+	return g_strdup (location);

Maybe you can skip the intermediate variables as they are not used?

I think that's it. Sorry if I said something very stupid, still under the effects of jet lag :)

(In reply to comment #13)

> +       EphyEmbedNetState estate = EPHY_EMBED_STATE_UNKNOWN;
> 
> Isn't it spelled state?
> 

Ok, state is already used. Still... :)

Also, I wonder what we should do about the signals that EphyEmbed inherits from GtkMozEmbed, redefining them in the other backends is kind of ugly.

(In reply to comment #13)
> About that, at this
> point of my life I kind of think that using tabs for indentation makes no
> sense, but I guess you think being consistent with the rest of epiphany is more
> important. Being a separate module inside ephy I don't think it's that
> relevant, but let's do it anyway...

Personally I've come to not like tabs for indentation either; so IMHO it would be okay to change our code style to make new files no-tabs-allowed, 2-space indented.

(In reply to comment #15)
> Also, I wonder what we should do about the signals that EphyEmbed inherits from
> GtkMozEmbed, redefining them in the other backends is kind of ugly.

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.)

(In reply to comment #16)
> Personally I've come to not like tabs for indentation either; so IMHO it would
> be okay to change our code style to make new files no-tabs-allowed, 2-space
> indented.

2? I'd go with 4, seems to much more canonical, but you are the boss.

> 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.)
> 

Sounds good to me, if you want to write your plans with a bit more detail I can help you to implement it.

Thanks for the review Xan!
I think I'll wait for the signals connecting code to be moved, to update my patch. Also, I'll be glad to help with it too! Let's open a bug for that.

*** Bug 483064 has been marked as a duplicate of this bug. ***

Ok, I've made the suggested cleanups to the patch and will commit it minus the gecko specific signals, which still needs work. I will commit this for now and we can keep working on bug 479919.

Created attachment 96783 [details] [review]
[1/1] Implement several missing methods.


Based on the patch by Cosimo Cecchi.

This adds the stubs for proper net status notification,
implements get_title, get_location and workarounds the bug
in webkit that will make pages not load images if the protocol
is not specified in the url (it will only work for http though).
---
 embed/webkit/webkit-embed.cpp |  176 ++++++++++++++++++++++++++++-------------
 1 files changed, 122 insertions(+), 54 deletions(-)

Attaching patch as committed, closing the bug. Thanks!