Bug 15006 - webkitdirs.pm contains too much code duplication for Qt and Gtk related stuff
Summary: webkitdirs.pm contains too much code duplication for Qt and Gtk related stuff
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-19 07:28 PDT by Oleg Sukhodolsky
Modified: 2007-10-14 13:57 PDT (History)
3 users (show)

See Also:


Attachments
refactoring of webkitdirs.pm (7.05 KB, patch)
2007-08-19 07:38 PDT, Oleg Sukhodolsky
darin: review+
Details | Formatted Diff | Diff
the patch updated to feature branch (6.31 KB, patch)
2007-10-10 08:57 PDT, Oleg Sukhodolsky
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Sukhodolsky 2007-08-19 07:28:09 PDT
buildQMakeGdkProject()/buildQMakeProject() are almost identical to each other.
The same is treu for isQt() and isGdk().
Comment 1 Oleg Sukhodolsky 2007-08-19 07:38:34 PDT
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 :)
Comment 2 George Staikos 2007-08-19 20:26:10 PDT
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.
Comment 3 Adam Treat 2007-08-19 20:37:16 PDT
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?
Comment 4 Oleg Sukhodolsky 2007-08-19 23:08:56 PDT
(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.

Comment 5 Oleg Sukhodolsky 2007-08-19 23:12:35 PDT
(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.

Comment 6 Holger Freyther 2007-08-24 07:42:36 PDT
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.
Comment 7 George Staikos 2007-08-24 13:51:22 PDT
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.
Comment 8 Oleg Sukhodolsky 2007-08-24 15:37:13 PDT
(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?

Comment 9 Darin Adler 2007-10-04 09:06:54 PDT
Comment on attachment 16021 [details]
refactoring of webkitdirs.pm

Looks good. r=me
Comment 10 Oleg Sukhodolsky 2007-10-10 08:57:53 PDT
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.
Comment 11 Mark Rowe (bdash) 2007-10-14 04:52:27 PDT
Landed in r26586.
Comment 12 George Staikos 2007-10-14 09:16:29 PDT
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.
Comment 13 Mark Rowe (bdash) 2007-10-14 13:57:20 PDT
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?