GNOME Bugzilla – Bug 694659
Create ephy-web-dom-utils for sharing DOM bindings code between WK1 and WK2
Last modified: 2013-02-28 17:10:30 UTC
At this moment code related to DOM bindings is copy-pasted in web extension in order to be used from WK2 (so far only the code for checking modified forms is copied). We should have a common place to share this code and avoid copy-pasting.
Created attachment 237348 [details] [review] Patch
Created attachment 237417 [details] [review] Patch updated Remove MIN_INPUT_LENGTH from embed/ephy-web-view.c as I forgot to remove it in the previous patch.
Review of attachment 237417 [details] [review]: ::: embed/web-extension/Makefile.am @@ +11,3 @@ + ephy-web-extension.h \ + $(top_srcdir)/lib/ephy-web-dom-utils.c \ + $(top_srcdir)/lib/ephy-web-dom-utils.h Indentation seems to be off here? ::: embed/web-extension/ephy-web-extension.c @@ +37,1 @@ #define MIN_INPUT_LENGTH 50 Can't you just remove this?
Created attachment 237547 [details] [review] Patch updated fixing issues detected in the review
Review of attachment 237547 [details] [review]: ::: embed/web-extension/ephy-web-extension.c @@ +37,1 @@ #define MIN_INPUT_LENGTH 50 So you left the constant here and hardcoded the number in ephy-web-dom-utils.c? I'm confused. It would seem you need to remove it here and just use it in the implementation.
(In reply to comment #5) > Review of attachment 237547 [details] [review]: > > ::: embed/web-extension/ephy-web-extension.c > @@ +37,1 @@ > #define MIN_INPUT_LENGTH 50 > > So you left the constant here and hardcoded the number in ephy-web-dom-utils.c? > I'm confused. It would seem you need to remove it here and just use it in the > implementation. I'm confused too. I know that I should remove that line and I don't know how I forgot it. Sorry next time I'll try to be more careful.
Created attachment 237564 [details] [review] Patch updated
Review of attachment 237564 [details] [review]: Looks good, thanks!
(In reply to comment #8) > Review of attachment 237564 [details] [review]: > > Looks good, thanks! Please push it by yourself as I don't have permissions at git.gnome.org. Thanks :-)
Created attachment 237614 [details] [review] Patch renaming method It renames ephy_web_dom_has_modified_forms to ephy_web_dom_utils_has_modified_forms in order to match with the file name.
Review of attachment 237614 [details] [review]: Thanks, I'll push this now.