GNOME Bugzilla – Bug 676484
ephy-downloads: decide_action_from_mime() is a mess
Last modified: 2012-06-14 11:45:55 UTC
- mime_description is leaked - the final comment is confusing (it talks about DOWNLOAD_ACTION_DOWNLOAD but this is never used. Does it mean that this is the default value of action?) - the two final assignations could be joined into one, but then it would make sense to set action to EPHY_DOWNLOAD_ACTION_BROWSE_TO as its default value instead? Rewrite the whole thing in one go, since right now it looks all patched.
(In reply to comment #0) > - mime_description is leaked > - the final comment is confusing (it talks about DOWNLOAD_ACTION_DOWNLOAD but > this is never used. Does it mean that this is the default value of action?) It means the action is not changed to OPEN because there was no helper app. Then the code proceeds to change it to BROWSE_TO, which makes sense. > - the two final assignations could be joined into one, but then it would make > sense to set action to EPHY_DOWNLOAD_ACTION_BROWSE_TO as its default value > instead? So what really does not make sense there is to change the mime_type when you are not going to use it anymore right? I think I'd just do if (!mime_type || !helper_app) action = BROWSE_TO; and be done with it. Just set the description to NULL at first and always g_free it at the end regardless. That's what I'd do anyway. > > Rewrite the whole thing in one go, since right now it looks all patched.
Created attachment 215079 [details] [review] e-download: simplify decide_action_from_mime Remove some leftover logic from older versions of this function. Add a gtk-doc comment explaining how the function works.
*** Bug 677440 has been marked as a duplicate of this bug. ***
Review of attachment 215079 [details] [review]: ::: embed/ephy-download.c @@ +239,2 @@ */ + if (mime_description == NULL || helper_app == NULL) mime_description is used here, but was freed already. helper_app is still leaked. We don't need the mime_description, since g_app_info_get_default_for_type() uses the content_type
Created attachment 215623 [details] [review] Updated patch This is a merge of the patch I posted in bug #677440 and Diego's patch
Review of attachment 215623 [details] [review]: Code looks fine, just a nitpick. ::: embed/ephy-download.c @@ +204,3 @@ + * + * Returns: a default action for @download + **/ I don't really like full gtk-doc stuff for private methods. IIRC the scanner will parse them, which is annoying, and it's also somewhat confusing for humans reading the code (since at first it seems like a public method if you don't read carefully).
Me neither, that was part of diego's patch, do you want me to remove it or convert it to a normal comment to clarify what the method does?
(In reply to comment #7) > Me neither, that was part of diego's patch, do you want me to remove it or > convert it to a normal comment to clarify what the method does? Normal comment is Ok.
Created attachment 216268 [details] [review] Updated patch
Comment on attachment 216268 [details] [review] Updated patch OK.