Created attachment 341400 [details] [review]
prefs-dialog: Expose homepage-url setting in preferences dialog

Created attachment 341401 [details] [review]
Show home button in the toolbar when homepage-url setting is enabled

Created attachment 341402 [details]
Screenshot

(In reply to Carlos Garcia Campos from comment #3)
> Created attachment 341402 [details]
> Screenshot

It seems more natural to have the New Tab button as the last one, after the Home button. That's usually the case when I see tabs and a new button (gnome-terminal, other browsers) and I usually see that when there's Home/New (bugzilla Home/New at the top of the page).

Thoughts?

The new tab button is really supposed to move to the tab bar, but nobody working on the tabs bar. :/ I agree it would probably fit better on the end....

I'd also suggest we change the text "Default new tab page" -> "Most visited websites"

Also let's say "homepage" instead of "home page"

Review of attachment 341400 [details] [review]:

::: src/resources/prefs-dialog.ui
@@ +69,3 @@
+                            <child>
+                              <object class="GtkRadioButton" id="new_tab_homepage_radiobutton">
+                                <property name="label" translatable="yes">_Default new tab page</property>

Careful with the mnemonics, D is already used for the download folder so you'll need to pick a different one.

@@ +76,3 @@
+                            <child>
+                              <object class="GtkRadioButton" id="blank_homepage_radiobutton">
+                                <property name="label" translatable="yes">_Blank page</property>

Is there an underscore here that splinter is not showing me, or are you missing the mnemonic?

Review of attachment 341401 [details] [review]:

Consider flipping the position with new tab... but then got to make sure the space that would be created disappears when the button is removed.

I wonder if we really need the button at all.

(In reply to Iulian Radu from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > Created attachment 341402 [details]
> > Screenshot
> 
> It seems more natural to have the New Tab button as the last one, after the
> Home button. That's usually the case when I see tabs and a new button
> (gnome-terminal, other browsers) and I usually see that when there's
> Home/New (bugzilla Home/New at the top of the page).
> 
> Thoughts?

It's fine with me.

(In reply to Michael Catanzaro from comment #5)
> The new tab button is really supposed to move to the tab bar, but nobody
> working on the tabs bar. :/ I agree it would probably fit better on the
> end....
> 
> I'd also suggest we change the text "Default new tab page" -> "Most visited
> websites"

I used "new tab page" because "most visited" is not true in incognito mode. The default new tab page is overview or incognito, and other browsers use new tab page as well.

(In reply to Michael Catanzaro from comment #8)
> Review of attachment 341401 [details] [review] [review]:
> 
> Consider flipping the position with new tab... but then got to make sure the
> space that would be created disappears when the button is removed.

Sure.

> I wonder if we really need the button at all.

I assume people wanting a homepage also want a home button, the feature is currently hidden, and the shortcut is not even in the shortcuts window, so how are users supposed to know they have to press ALT+Home?

(In reply to Michael Catanzaro from comment #6)
> Also let's say "homepage" instead of "home page"

Ok.

(In reply to Michael Catanzaro from comment #7)
> Review of attachment 341400 [details] [review] [review]:
> 
> ::: src/resources/prefs-dialog.ui
> @@ +69,3 @@
> +                            <child>
> +                              <object class="GtkRadioButton"
> id="new_tab_homepage_radiobutton">
> +                                <property name="label"
> translatable="yes">_Default new tab page</property>
> 
> Careful with the mnemonics, D is already used for the download folder so
> you'll need to pick a different one.

Oh.

> @@ +76,3 @@
> +                            <child>
> +                              <object class="GtkRadioButton"
> id="blank_homepage_radiobutton">
> +                                <property name="label"
> translatable="yes">_Blank page</property>
> 
> Is there an underscore here that splinter is not showing me, or are you
> missing the mnemonic?

There is, _Blank page.

> the shortcut is not even in the shortcuts window, so
> how are users supposed to know they have to press ALT+Home?

We should add it there as well.

The following fix has been pushed:
c1ff13e header-bar: Fix memory leak when changing homepage

Created attachment 344501 [details] [review]
header-bar: Fix memory leak when changing homepage