Bug 15006 – webkitdirs.pm contains too much code duplication for Qt and Gtk related stuff

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Oleg Sukhodolsky]

buildQMakeGdkProject()/buildQMakeProject() are almost identical to each other.
The same is treu for isQt() and isGdk().

[Oleg Sukhodolsky]

Created attachment 16021 [details]
refactoring of webkitdirs.pm

To make buildQMakeGdkProject()/buildQMakeQtProject() as similar as possible I've decided to only use "qmake" for build (before gtk port tried to use qmake-qt4 first).  But since it is possible now to configure qmake which will be used with "--qmake" cli option, it should not be a big problem.

Also I've decided to not place the check for directory ($project) for which build is called in common area since I see no advantages from this.

Note, that I have not tested this with Qt build, freyther has agreed to do this for me (thanks in advance btw :)

[George Staikos]

I'm a bit afraid that this will limit flexibility and I don't see a good reason why they should be using the exact same codepath.

[Adam Treat]

Would probably be good to get rid of the whitespace changes.  Also, I think this patch reverts 25146 that zecke said was necessary for those folks who don't build qt directly.... no?

[Oleg Sukhodolsky]

(In reply to comment #2)
> I'm a bit afraid that this will limit flexibility and I don't see a good reason
> why they should be using the exact same codepath.

For now the codepaths is similar and this is the motivation for this refactoring. 
And the current change doesn't mean that you can not change this in future :)
So the patch is to express that NOW build process for Qt port is the same as for 
Gtk one.

[Oleg Sukhodolsky]

(In reply to comment #3)
> Would probably be good to get rid of the whitespace changes.  Also, I think
> this patch reverts 25146 that zecke said was necessary for those folks who
> don't build qt directly.... no?

as far as I understand 25146 is about passing debug/release to qmake.   If so then
the patch doesn't remove it.

[Holger Freyther]

pmax: As of now it is (almost) the same code, you get flexibility through your --qmakearg command line parameter and if you really need to add Qt specific options that doesn't make sense for Gtk+ then we are always able to separate the implementations again.

I have tested the patch and WebKit/Qt continued to compile here. I agree with the change as it strives for simplification and unification.

[George Staikos]

This still doesn't answer what the reason for the change is besides an artificial desire to have to different things use the same function even though there is potential for them to need different behavior in the near future.

[Oleg Sukhodolsky]

(In reply to comment #7)
> This still doesn't answer what the reason for the change is besides an
> artificial desire to have to different things use the same function even though
> there is potential for them to need different behavior in the near future.

I think most likely both builds have a lot of in common: they  should be able to 
choose qmake binary (--qmake), set some common params, read params from command 
line (--qmakearg), handle WEBKIT_FULLBUILD, run qmake and checks its result, and, 
perhaps, something else.

So, current patch it attempt to write this in the code.  It might be not perfect
but it is good enough for current situation.  Also every time someone will add 
something to either Qt or Gtk build, (s)he will be forced (by code) to think 
about adding this to another build.

Does this make sense for you?

[Darin Adler]

Comment on attachment 16021 [details]
refactoring of webkitdirs.pm

Looks good. r=me

[Oleg Sukhodolsky]

Created attachment 16613 [details]
the patch updated to feature branch

I have update the patch so it could be easily applied to feature branch.
And tested it with gtk build.

[Mark Rowe (bdash)]

Landed in r26586.

[George Staikos]

Now that this patch has gone in, please stop changing the Qt build system.  It is causing major headaches for me as I have to constantly merge changes on my side.  If there is a need to change the gtk build system please do it without changing the Qt one, or at least don't check it in when the Qt developers give negative review.

[Mark Rowe (bdash)]

To be fair, you didn't give a negative review, you said you didn't see a need for the change.  Others disagreed and took the time to write and review a patch.  How are we to know that a patch will cause merge troubles on your private branches unless you say as much?