Created attachment 314654 [details] [review]
encoding-dialog.ui: Correct visual bug on resizing.

Created attachment 314655 [details] [review]
encoding-dialog.ui: Manually unbloat.

Created attachment 314656 [details] [review]
encoding-dialog.ui: Kill a level of GtkBox.

Created attachment 314658 [details] [review]
encoding-dialog.ui: Make unresizable.

Created attachment 314662 [details] [review]
EphyEncodingDialog: Use template.

So. This patch does what is was designed for: use a template that makes the EphyEncodingDialog class inherit directly from GtkDialog. It permits to remove the EphyEmbedDialog thing, a quite complex way for maintaining a ref to the current page.

It doesn’t correct the bugs of the dialog itself (like clicking on “Use a different encoding:” radio that does nothing), nor does it try to put all in the GtkBuilder file. All that are for patches coming after.

The thing is that I’m not understanding all that is in it (weak_pointer_what?), and I’m not used enough to C to be sure they aren’t any problem there. So, can you please check all that? What can I improve, correct, do better?

Created attachment 314663 [details] [review]
Remove EphyEmbedDialog class.

Created attachment 314666 [details] [review]
Remove EphyDialog class.

Has the lib/ folder a special status? if not, this class could also now be removed.

(But if so, ephy-web-dom-utils.* also can.)

(oh, no, sorry, used at another level of folders)

Review of attachment 314654 [details] [review]:

OK

Review of attachment 314655 [details] [review]:

OK

Review of attachment 314656 [details] [review]:

OK

Review of attachment 314658 [details] [review]:

It'd be better to change the sizes in a separate patch. (I presume you're changing the sizes intentionally.)

Review of attachment 314662 [details] [review]:

Fun patch :)

Hm, I know I told you this was a good idea, but now that I see the result, I don't think I like the extra complexity we're adding to EphyEncodingDialog. I don't like inheritance as a rule, but it's true that the functionality of EphyEmbedDialog is quite clearly separated from that of EphyEncodingDialog. That said, I think this will be fine, because I think we can remove most of the complexity of EphyEmbedDialog that you've added to EphyEncodingDialog in this patch.

* Is the embed property ever used? If it's not needed, don't make it a property, to make this simpler.
* Is the parent property really needed? Surely not: just use gtk_window_set_transient_for(), perhaps in ephy_encoding_dialog_new().

So basically I think you probably don't need the properties... I haven't checked thoroughly, though.

I would also reword the first line of the commit message more descriptive. The fact that a GtkTemplate is used is not very interesting compared to the rest of what you're doing here.

::: src/ephy-encoding-dialog.c
@@ +165,2 @@
 static void
+unset_embed (EphyEncodingDialog *dialog)

Why would this ever happen? The encoding dialog is modal, so I don't think an embed should ever disappear while the dialog is open, and you don't need this function.

BUT: I might be wrong if it's possible for an embed (tab) to ever close itself while the encodings dialog is open. I think not, because I think modal dialogs are implemented with a nested main loop, so only events from the encodings dialog should be processed while it is open. Please ask Carlos, he usually knows about such things. :)

@@ +168,1 @@
 	if (dialog->priv->embed != NULL)

If I'm right, and the embed can never disappear while the encoding dialog is open, then you don't need to check for null here (because you'll get rid of the weak pointer).

@@ +170,3 @@
+		EphyEmbed **embedptr;
+
+		g_signal_handlers_disconnect_by_func (ephy_embed_get_web_view (dialog->priv->embed),

This can be moved to dispose.

@@ +175,3 @@
+
+		embedptr = &dialog->priv->embed;
+		g_object_remove_weak_pointer (G_OBJECT (dialog->priv->embed),

If I'm right, and the embed can never disappear while the encoding dialog is open, then you don't need this weak pointer.

@@ +181,3 @@
+
+static void
+ephy_encoding_dialog_set_embed (EphyEncodingDialog *dialog,

Since the dialog is modal, I don't see any value in allowing the embed to be changed. I would drop this function. Or at least make the property construct-only, which would allow you to get rid of the weak pointer.

@@ +200,3 @@
 	sync_encoding_against_embed (dialog);
+
+	g_object_notify (G_OBJECT (dialog), "embed");

This is a double-notify, which we should take the opportunity to fix. If you notify manually, you should use G_PARAM_EXPLICIT_NOTIFY when declaring the property. In this case you can just delete the manual notify.

I think we have this problem throughout our codebase, though. I'm surprised it doesn't cause bugs; I guess because it's rare to listen on notifies.

@@ +215,2 @@
 static void
+unset_parent (EphyEncodingDialog *dialog)

The window can't disappear when the dialog is open. You don't need this function.

@@ +217,2 @@
 {
+	if (dialog->priv->window != NULL)

Don't need this check, because you don't need the weak pointer.

@@ +222,1 @@
+		g_signal_handlers_disconnect_by_func (dialog->priv->window,

This can simply be moved to dispose. (Not finalize!)

@@ +226,2 @@
+		windowptr = &dialog->priv->window;
+		g_object_remove_weak_pointer (G_OBJECT (dialog->priv->window),

And this weak pointer, you shouldn't need.

@@ +236,3 @@
+	EphyWindow **windowptr;
+
+/*	unset_parent (dialog); TODO or create construct only? */

Should be construct-only, indeed.

@@ +241,3 @@
 
+	g_signal_connect (G_OBJECT (window), "notify::active-child",
+	                  G_CALLBACK (sync_active_tab), dialog);

Should be able to delete sync_active_tab.

@@ +246,3 @@
 
+	windowptr = &dialog->priv->window;
+	g_object_add_weak_pointer (G_OBJECT (dialog->priv->window),

...then you don't need this weak pointer either...

@@ +251,3 @@
 	sync_active_tab (window, NULL, dialog);
+
+	g_object_notify (G_OBJECT (dialog), "parent-window");

Another double-notify.

@@ +435,3 @@
 
+	unset_embed (dialog);
+	unset_parent (dialog);

Best do this in dispose, not finalize. I think it doesn't matter in this case, but it's hard to reason about reference cycles if finalize does more than free memory. I know I asked you to remove these functions, but you'll need to move the signal disconnection somewhere, and I think dispose is a safer place for that, too. (Carlos, do you agree?)

::: src/ephy-encoding-menu.c
@@ +288,3 @@
 		menu->priv->dialog = ephy_encoding_dialog_new
 					(menu->priv->window);
+		gtk_window_set_transient_for (GTK_WINDOW (menu->priv->dialog),

So you also added a call to gtk_window_set_transient_for... what is the parent-window property for, then?

(In reply to Michael Catanzaro from comment #14)
> * Is the parent property really needed? Surely not: just use
> gtk_window_set_transient_for(), perhaps in ephy_encoding_dialog_new().
> 
> ::: src/ephy-encoding-menu.c
> @@ +288,3 @@
>  		menu->priv->dialog = ephy_encoding_dialog_new
>  					(menu->priv->window);
> +		gtk_window_set_transient_for (GTK_WINDOW (menu->priv->dialog),
> 
> So you also added a call to gtk_window_set_transient_for... what is the
> parent-window property for, then?

Sorry about these stale comments to be ignored; I did the review somewhat out-of-order and did not take the time to review my comments once more at the end, like I should have.

Review of attachment 314663 [details] [review]:

+1, but dependent on the 'needs-work' patch above.

Review of attachment 314666 [details] [review]:

+1, also dependent on the 'needs-work' patch above.

(In reply to Michael Catanzaro from comment #14)
> * Is the embed property ever used? If it's not needed, don't make it a
> property, to make this simpler.
> * Is the parent property really needed? Surely not: just use
> gtk_window_set_transient_for(), perhaps in ephy_encoding_dialog_new().
> 
> So basically I think you probably don't need the properties... I haven't
> checked thoroughly, though.

To be clear: I think it suffices to keep these as members of the instance struct, just no need to have properties for them.

(In reply to Arnaud B. from comment #5)
> The thing is that I’m not understanding all that is in it
> (weak_pointer_what?), and I’m not used enough to C to be sure they aren’t
> any problem there. So, can you please check all that? What can I improve,
> correct, do better?

The weak pointers are used to watch for objects being deleted out from under us. They're used by EphyEmbedDialog because it doesn't ref the embed or the parent window; instead, it uses the weak pointers to prepare for them to be deleted. Without the weak pointer, the pointer held by EphyEmbedDialog would never be set to NULL when the corresponding object is destroyed, and we'd have a dangerous dangling pointer. The risk is that you have to be careful to remove the weak pointer at the right time, else you wind up with bad problems like the one fixed in commit 3596b8e88b0b98a5133c609124458d6d0d968062.

Docs: https://developer.gnome.org/gobject/2.47/gobject-The-Base-Object-Type.html#g-object-add-weak-pointer

Also useful (but not used here): https://developer.gnome.org/gobject/2.47/gobject-The-Base-Object-Type.html#g-object-weak-ref

(In reply to Arnaud B. from comment #7)
> Created attachment 314666 [details] [review] [review]
> Remove EphyDialog class.
> 
> Has the lib/ folder a special status? if not, this class could also now be
> removed.

You can remove unused things from it! It's special in that it can't include things from other directories. The dependencies are:

src <- embed <- lib

i.e. files in src/ can include anything, and files in embed/ can include files in lib/, but files in lib/ cannot include files in src/ or embed/.

(In reply to Michael Catanzaro from comment #14)
> @@ +251,3 @@
>  	sync_active_tab (window, NULL, dialog);
> +
> +	g_object_notify (G_OBJECT (dialog), "parent-window");
> 
> Another double-notify.

Well I'm wrong, according to https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html GObject actually checks to see if we emit notify manually when the setter is called, and only does so if we didn't. Still, it's redundant.

(In reply to Michael Catanzaro from comment #13)
> It'd be better to change the sizes in a separate patch. (I presume you're
> changing the sizes intentionally.)

Badly presumed. ^(^ Well, in fact, making the dialog unresizable makes it change the way size is calculated, so the patch I pushed changes the size a little; but the height/width inversion that I corrected before pushing was just a typo.

Correction from Carlos: "Modal dialogs use a nested main loop but with the default main context. So, you won't be able to click on the close tab button, and the shortcut will be handled by the modal window, but window.close() could be called on the main window, and the view will be destroyed so the tab will be closed."

So that means we do need the weak pointer to the EphyEmbed. Still, this will probably never happen to anyone ever, so I think it's fine to just destroy the EphyEncodingsDialog in the insane chance this occurs.

Created attachment 314782 [details] [review]
EphyEncodingDialog: Inherit from GtkDialog, not from EphyEmbedDialog.

Second round. I made the ‘parent-window’ property a construct only (as it is set at construction, doesn’t change, and does work like maintaining the sync) and removed the ‘embed’ one.

Review of attachment 314782 [details] [review]:

Much better.

::: src/ephy-encoding-dialog.c
@@ +65,3 @@
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE (EphyEncodingDialog, ephy_encoding_dialog, GTK_TYPE_DIALOG)

Adding this just to get rid of it later? OK....

@@ +168,2 @@
+	embedptr = &dialog->priv->embed;
+	g_object_remove_weak_pointer (G_OBJECT (dialog->priv->embed),

Since this function is called from dispose, which can be called multiple times, it has to be safe against being called multiple times in a row. So you need to check that embedptr is not NULL before calling g_object_remove_weak_pointer.

@@ +174,2 @@
 static void
+ephy_encoding_dialog_set_embed (EphyEncodingDialog *dialog)

Hm, it's named like a setter, but you don't pass in the EphyEmbed... I think it would be best to merge this function into ephy_encoding_dialog_sync_embed().

@@ +415,3 @@
 	{
+	case PROP_PARENT_WINDOW:
+		dialog = EPHY_ENCODING_DIALOG (object);

It's idiomatic to split this into a function, ephy_encoding_dialog_set_parent_window().

Created attachment 314958 [details] [review]
EhpyEncodingDialog: Inherit from GtkDialog, not from EphyEmbedDialog.

Third round. Corrected a crash when closing the dialog during the reload of the webpage. That was partly linked with a multi-call of the dispose function, and these cases should be handled now.

I won’t touch to the Private structure at this patch, so yes, use G_DEFINE_TYPE_WITH_PRIVATE for now.

I renamed ephy_encoding_dialog_set_embed to ephy_encoding_dialog_init_embed, hoping it’s better for C coders (I honestly don’t mind). Also created ephy_encoling_dialog_set_parent_window as requested.

Created attachment 314959 [details] [review]
EphyEncodingDialog: Make final class.

Review of attachment 314958 [details] [review]:

::: src/ephy-encoding-dialog.c
@@ +198,3 @@
+{
+	ephy_encoding_dialog_unset_embed (dialog);
+	ephy_encoding_dialog_init_embed (dialog);

This isn't really a great function name, either. What I would do is remove ephy_encoding_dialog_init_embed(), and move all the code into ephy_encoding_dialog_sync_embed().

@@ +390,3 @@
+		                                      G_CALLBACK (embed_net_stop_cb),
+		                                      dialog);
+		ephy_encoding_dialog_unset_embed (dialog);

I'd move this disconnect into _unset_embed()

@@ +465,3 @@
+	object_class->set_property = ephy_encoding_dialog_set_property;
+	object_class->get_property = ephy_encoding_dialog_get_property;
+	object_class->dispose      = ephy_encoding_dialog_dispose;

I prefer not to line up the assignment operators.