GNOME Bugzilla – Bug 691416
Don't change is_blank property depending on the title
Last modified: 2013-01-11 09:08:09 UTC
Created attachment 233070 [details] [review] Patch In WebKit1 the notify::title signal is mitted twice for every load, first with NULL to reset the title and then with the actual title. A NULL title doesn't neccessarily means the page is about:blank, and the property is updated already by ephy_web_view_set_address.
Review of attachment 233070 [details] [review]: ::: embed/ephy-web-view.c @@ -1182,3 @@ return g_strdup (address + 7); - else if (!strcmp (address, EPHY_ABOUT_SCHEME":plugins")) - return g_strdup (_("Plugins")); This seems to be unrelated to the patch? I'd guess it was done to translate the page title. I guess we could do it through <title> in the about handler, but do it in a different patch. @@ -1221,3 @@ - } else if (priv->is_blank) { - g_free (title); - title = g_strdup (EMPTY_PAGE); What the code path in set_address does not seem to do is to set the title of about:blank to EMPTY_PAGE? Or are you also relying on what happens in location_changed?
(In reply to comment #1) > Review of attachment 233070 [details] [review]: > > ::: embed/ephy-web-view.c > @@ -1182,3 @@ > return g_strdup (address + 7); > - else if (!strcmp (address, EPHY_ABOUT_SCHEME":plugins")) > - return g_strdup (_("Plugins")); > > This seems to be unrelated to the patch? I'd guess it was done to translate the > page title. I guess we could do it through <title> in the about handler, but do > it in a different patch. I assumed this was a hack to workaround an issue related to this fix. The plugins about handler already has a translatable title: _("Installed plugins") > @@ -1221,3 @@ > - } else if (priv->is_blank) { > - g_free (title); > - title = g_strdup (EMPTY_PAGE); > > What the code path in set_address does not seem to do is to set the title of > about:blank to EMPTY_PAGE? Or are you also relying on what happens in > location_changed? No, for about:blank title will be NULL and get_title_from_address() will return NULL too, so EMPTY_PAGE will be set.
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 233070 [details] [review] [details]: > > > > ::: embed/ephy-web-view.c > > @@ -1182,3 @@ > > return g_strdup (address + 7); > > - else if (!strcmp (address, EPHY_ABOUT_SCHEME":plugins")) > > - return g_strdup (_("Plugins")); > > > > This seems to be unrelated to the patch? I'd guess it was done to translate the > > page title. I guess we could do it through <title> in the about handler, but do > > it in a different patch. > > I assumed this was a hack to workaround an issue related to this fix. The > plugins about handler already has a translatable title: _("Installed plugins") Ah right, no idea what this was for then. Just remove it in a different patch please. > > > @@ -1221,3 @@ > > - } else if (priv->is_blank) { > > - g_free (title); > > - title = g_strdup (EMPTY_PAGE); > > > > What the code path in set_address does not seem to do is to set the title of > > about:blank to EMPTY_PAGE? Or are you also relying on what happens in > > location_changed? > > No, for about:blank title will be NULL and get_title_from_address() will return > NULL too, so EMPTY_PAGE will be set. I see, so in the end it gets a NULL title because ephy_string_get_host_name will return NULL for about: pages. Makes sense, I guess, although reading that code I guess we could just add about: to file: in the list of things that we know won't have a hostname :)
Comment on attachment 233070 [details] [review] Patch OK to commit in two separate patches as suggested in the comments.