GNOME Bugzilla – Bug 705180
Redundant code on the different error pages
Last modified: 2014-02-06 17:34:33 UTC
The different error pages that Epiphany is showing depending on the type of the error (like EPHY_WEB_VIEW_ERROR_PAGE_HTTP_ERROR, EPHY_WEB_VIEW_ERROR_PAGE_CRASH, EPHY_WEB_VIEW_ERROR_PROCESS_CRASH), have all the same HTML code on different files (with some slight style modifications on process_crash.html). Keeping a single error.html file, adding an extra "class" parameter to keep the current minor aspect differences will make easier to maintain and evolve the error pages. I'd be also interesting to add more actions to the existing 'Try again' button, like buttons to 'Go Back' and 'Go Home' (which is the overview if not a custom homepage has been defined).
Created attachment 250510 [details] [review] Merge the different error pages and add new home/back buttons
Review of attachment 250510 [details] [review]: It works properly and looks great overall. Just needed to fix some leaks and minors nits. ::: embed/ephy-web-view.c @@ +2457,3 @@ g_strdelimit (lang, "_-@", '\0'); + Nit: This new line is not needed. @@ +2458,3 @@ + + home_address = g_settings_get_string (EPHY_SETTINGS_MAIN, EPHY_PREFS_HOMEPAGE_URL); You should free this at the end of the method. @@ +2460,3 @@ + home_address = g_settings_get_string (EPHY_SETTINGS_MAIN, EPHY_PREFS_HOMEPAGE_URL); + if (home_address == NULL || home_address[0] == '\0') + home_address = "ephy-about:overview"; You should free home_address before setting the new value (it could be empty). As you have to free it also at the end of the method I'd use g_strdup() here. @@ +2472,3 @@ + _("Go Back")); + else + I guess that this will crash when trying to free the variable later. @@ +2475,3 @@ + + html_file = ephy_file ("error.html"); + button_reload = g_strdup_printf ("<button onclick=\"javascript:load_anyway()\">%s</button>", I guess in this case you don't need to do a copy (g_strdup()). A "const gchar *" would be enough. @@ +2523,1 @@ + custom_class = g_strdup ("process-crash"); Ditto. @@ +2545,3 @@ _ephy_web_view_update_icon (view); + Nit: This new line is not needed. @@ +2560,3 @@ g_free (msg_title); g_free (msg); + g_free (custom_class); This won't be needed if you use a "const gchar *" as explained before.
Created attachment 250633 [details] [review] Merge the different error pages and add new home/back buttons Thanks for the revision. Suggestions and comments applied to the patch and resent.
Review of attachment 250633 [details] [review]: You shouldn't mix the refactoring you're doing with adding new buttons. Please file a separate bug to discuss whether we want to add new buttons and add only the refactoring bits here. ::: embed/ephy-web-view.c @@ +2467,3 @@ + _("Try again")); + + if (!g_settings_get_boolean (EPHY_SETTINGS_LOCKDOWN,EPHY_PREFS_LOCKDOWN_HISTORY)) Missing space after the comma.
Created attachment 250703 [details] [review] Merge the different error pages Thanks for the revision. It makes all the sense to submit the changes split into different patches, I have created https://bugzilla.gnome.org/show_bug.cgi?id=705352 for those actions
I'm sorry, I can't apply this against master.
Review of attachment 250703 [details] [review]: LGTM, just fixed some minor nits due to unneeded extra new lines. Pushed into kiosk-mode branch. We should provide a new patch rebased against master.
Created attachment 255866 [details] [review] [PATCH] Refactor redundant code of the different error pages Patch rebased to master branch.
Created attachment 263940 [details] [review] Refactor redundant code of the different error pages Refreshed rebase to current master.
Review of attachment 263940 [details] [review]: Looks good except the minor issue there. ::: embed/ephy-web-view.c @@ +1729,3 @@ + html_file = g_resources_lookup_data (EPHY_PAGE_TEMPLATE_ERROR, 0, NULL); + button_label = g_strdup (_("Try again")); This… @@ +1748,1 @@ button_label = g_strdup (_("Try again")); …is duplicated here.
Created attachment 268115 [details] [review] Refactor redundant code of the different error pages Rebased to current master, where the styles block has been already generalized to a separate file.
Review of attachment 268115 [details] [review]: ::: embed/ephy-web-view.c @@ +1750,3 @@ + html_file = g_resources_lookup_data (EPHY_PAGE_TEMPLATE_ERROR, 0, NULL); + button_label = g_strdup (_("Try again")); + custom_class = "network-error"; This should go into the EPHY_WEB_VIEW_ERROR_PAGE_NETWORK_ERROR case. @@ -1777,3 @@ LSB_DISTRIBUTOR); - - button_label = g_strdup (_("Reload Anyway")); Why does this go away? @@ -1785,3 @@ msg_title = g_strdup (_("Oops!")); msg = g_strdup (_("Something went wrong while displaying this page. Please reload or visit a different page to continue.")); - button_label = g_strdup (_("Reload Anyway")); Why does this go away?
Created attachment 268241 [details] [review] Refactor redundant code of the different error pages I have updated the patch with the commented changes. The motivation for moving away the strings - button_label = g_strdup (_("Reload Anyway")); was considering that the text for the three buttons could be also refactored, using the first "Try again" for all of them. As the action done by pressing the button it's the same on the three cases, and the differences between their text were quite subtle, I thought that it could be a good idea to use the same label for all the cases. However, this is probably better to be discussed on a different bug, so I have just left the original texts on this version of the patch. Thanks for the comments.
Review of attachment 268241 [details] [review]: Looks good, thanks.
Attachment 268241 [details] pushed as 936840d - Refactor redundant code of the different error pages