Bug 117545

Summary: [GTK] Remove legacy hack in CodeGeneratorGObject.pm
Product: WebKit Reporter: Diego Pino <dpino>
Component: BindingsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: buildbot, cdumez, cgarcia, commit-queue, rniwa, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion none

Description Diego Pino 2013-06-12 07:58:12 PDT
There's a piece of code that sets gtype to uint in case it's ushort. gtype is a value obtained from GetGValueTypeName(), which never returns ushort. The snippet is marked with a comment "FIXME: get rid of this glitch?"
Comment 1 Diego Pino 2013-06-12 08:01:27 PDT
Created attachment 204441 [details]
Patch
Comment 2 Diego Pino 2013-06-19 09:42:11 PDT
Created attachment 205012 [details]
Patch
Comment 3 WebKit Commit Bot 2013-07-22 11:07:42 PDT
Comment on attachment 205012 [details]
Patch

Rejecting attachment 205012 [details] from commit-queue.

dpino@igalia.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 4 Carlos Garcia Campos 2013-08-02 00:56:38 PDT
Comment on attachment 205012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205012&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:548
> +    } elsif ($attribute->signature->extendedAttributes->{"ImplementedBy"}) {
> +        my $implementedBy = $attribute->signature->extendedAttributes->{"ImplementedBy"};
> +        $implIncludes{"${implementedBy}.h"} = 1;

I wonder if we can get rid of this here, since this is already done a few lines above in this function.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:551
> +        push(@txtGetProps, "        g_value_set_$gtype(value, ${convertFunction}${getterFunctionName}(" . join(", ", @getterArguments) .  ")${postConvertFunction});\n");
> +    } else {
> +        push(@txtGetProps, "        g_value_set_$gtype(value, ${convertFunction}${getterFunctionName}(" . join(", ", @getterArguments) . ")${postConvertFunction});\n");

I wonder why these lines are duplicated, I know it's not introduced by you, but I think this can be simplified. If we can get rid of the implemented by check this could just be 

} else { 
    push(@txtGetProps, "        g_value_set_$gtype(value, ${convertFunction}${getterFunctionName}(" . join(", ", @getterArguments) . ")${postConvertFunction});\n");
}
Comment 5 Diego Pino 2013-08-02 01:05:47 PDT
Comment on attachment 205012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205012&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:551
>> +        push(@txtGetProps, "        g_value_set_$gtype(value, ${convertFunction}${getterFunctionName}(" . join(", ", @getterArguments) . ")${postConvertFunction});\n");
> 
> I wonder why these lines are duplicated, I know it's not introduced by you, but I think this can be simplified. If we can get rid of the implemented by check this could just be 
> 
> } else { 
>     push(@txtGetProps, "        g_value_set_$gtype(value, ${convertFunction}${getterFunctionName}(" . join(", ", @getterArguments) . ")${postConvertFunction});\n");
> }

Correct. I agree with these suggestions. I will cook a new patch with this changes, and check the script generates the same code.
Comment 6 Diego Pino 2013-08-02 06:55:45 PDT
Created attachment 208011 [details]
Patch
Comment 7 Build Bot 2013-08-02 08:05:11 PDT
Comment on attachment 208011 [details]
Patch

Attachment 208011 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1330178

New failing tests:
svg/batik/filters/feTile.svg
Comment 8 Build Bot 2013-08-02 08:05:13 PDT
Created attachment 208019 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 9 WebKit Commit Bot 2013-08-04 06:03:25 PDT
Comment on attachment 208011 [details]
Patch

Clearing flags on attachment: 208011

Committed r153697: <http://trac.webkit.org/changeset/153697>
Comment 10 WebKit Commit Bot 2013-08-04 06:03:29 PDT
All reviewed patches have been landed.  Closing bug.