Bug 105009 – [EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

• Component: WebKit EFL
• Status: RESOLVED DUPLICATE

[Krzysztof Czech]

Isolates some specific GTK code. It's a part of 105007 bug with sharing DumpRenderTree's implementation among other ports.

[Krzysztof Czech]

Created attachment 179460 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

[WebKit Review Bot]

Attachment 179460 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/g..." exit_code: 1
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 179462 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

[Mario Sanchez Prada]

Comment on attachment 179462 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

Looks good to me

[Martin Robinson]

Comment on attachment 179462 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

Instead of first splicing a file in the directory with #ifdefs I think it makes sense to move shared code to a shared directory, while keeping platform-specific methods or functions in the GTK+ directory.  This is the structure that many classes in WebCore use. It is based on the fact that the implementation of various class methods can be spread across compilation units.

[Krzysztof Czech]

I think you are right about keeping platform specific methods in GTK+ directory. 
Basically those methods where I used PLATFORM(GTK) ifdefs will be also shared. The only difference is call to DumpRenderTreeSupportGtk/Efl specific method.
I have already started implementing those missing bits from DumpRenderTreeSupporEfl.

[Martin Robinson]

(In reply to comment #6)
> I think you are right about keeping platform specific methods in GTK+ directory. 
> Basically those methods where I used PLATFORM(GTK) ifdefs will be also shared. The only difference is call to DumpRenderTreeSupportGtk/Efl specific method.
> I have already started implementing those missing bits from DumpRenderTreeSupporEfl.

Instead of sharing methods with #ifdefs, I'm suggesting creating new methods that hide the platform-specific behavior.

[Krzysztof Czech]

Created attachment 182961 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

[Krzysztof Czech]

(In reply to comment #7)
> (In reply to comment #6)
> > I think you are right about keeping platform specific methods in GTK+ directory. 
> > Basically those methods where I used PLATFORM(GTK) ifdefs will be also shared. The only difference is call to DumpRenderTreeSupportGtk/Efl specific method.
> > I have already started implementing those missing bits from DumpRenderTreeSupporEfl.
> 
> Instead of sharing methods with #ifdefs, I'm suggesting creating new methods that hide the platform-specific behavior.

I applied you suggestions. I tried to minimize usage of #ifdefs. Used them only in AccessibilityCallbacks.cpp. What do you think ?.

[Martin Robinson]

Comment on attachment 182961 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

It's hard to understand this change by itself. Perhaps it would be better to include the addition of the EFL bits here as well.

[Krzysztof Czech]

Created attachment 183769 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

[Krzysztof Czech]

I included additional EFL bits.
> (From update of attachment 182961 [details])
> It's hard to understand this change by itself. Perhaps it would be better to include the addition of the EFL bits here as well.

[Krzysztof Czech]

*** This bug has been marked as a duplicate of bug 105007 ***

[Eric Seidel (no email)]

Comment on attachment 183769 [details]
[EFL][DRT] Isolates GTK specific code in DumpRenderTree/gtk

Cleared review? from attachment 183769 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).