Bug 127412 - [iOS] Upstream changes in Tools/DumpRenderTree
Summary: [iOS] Upstream changes in Tools/DumpRenderTree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad All
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-22 00:02 PST by Andy Estes
Modified: 2014-01-26 23:32 PST (History)
8 users (show)

See Also:


Attachments
[iOS] Prepare for upstreaming DumpRenderTree changes (48.18 KB, patch)
2014-01-22 16:41 PST, Andy Estes
no flags Details | Formatted Diff | Diff
[iOS] Upstream changes in Tools/DumpRenderTree (388.76 KB, patch)
2014-01-24 11:15 PST, Andy Estes
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (541.57 KB, application/zip)
2014-01-24 12:32 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2014-01-22 00:02:34 PST
Multiple patches forthcoming.
Comment 1 Andy Estes 2014-01-22 16:41:04 PST
Created attachment 221924 [details]
[iOS] Prepare for upstreaming DumpRenderTree changes
Comment 2 Andy Estes 2014-01-22 17:07:08 PST
Committed r162573: <http://trac.webkit.org/changeset/162573>
Comment 3 Andy Estes 2014-01-24 11:15:18 PST
Created attachment 222124 [details]
[iOS] Upstream changes in Tools/DumpRenderTree
Comment 4 Simon Fraser (smfr) 2014-01-24 12:06:31 PST
Comment on attachment 222124 [details]
[iOS] Upstream changes in Tools/DumpRenderTree

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

> Tools/DumpRenderTree/AccessibilityTextMarker.h:61
> +#if PLATFORM(MAC) && SUPPORTS_AX_TEXTMARKERS

Don't think you need the PLATFORM(MAC) here any more.

> Tools/DumpRenderTree/AccessibilityTextMarker.h:81
> +#if PLATFORM(MAC) && SUPPORTS_AX_TEXTMARKERS

Ditto.

> Tools/DumpRenderTree/ios/Info.plist:12
> +	<string>com.apple.DumpRenderTree</string>

org.webkit?

> Tools/DumpRenderTree/ios/PerlSupport/IPhoneSimulatorNotification/Makefile.PL:1
> +# Copyright (C) 2009 Apple Inc. All rights reserved.

2014?

> Tools/DumpRenderTree/ios/PerlSupport/IPhoneSimulatorNotification/lib/IPhoneSimulatorNotification.pm:1
> +# Copyright (C) 2009 Apple Inc. All rights reserved.

2014?

> Tools/DumpRenderTree/ios/PerlSupport/Makefile:1
> +# Copyright (C) 2009 Apple Inc. All rights reserved.

2014

> Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:2
> + * Copyright (C) 2005, 2006, 2007, 2009 Apple Inc. All rights reserved.

2014

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:762
> +    // See also -[TabDocument _updateTextSize] in MobileSafari/TabDocument.m

Remove this comment.

> Tools/DumpRenderTree/mac/EventSendingController.mm:423
> +#if !PLATFORM(IOS)
>          flags |= NSControlKeyMask;
> +#else
> +        flags |= WebEventFlagMaskControl;
> +#endif

These are ugly. Can we just make some common #ifdefs or an enum?

> Tools/DumpRenderTree/mac/ObjCPlugin.m:133
> +#if PLATFORM(IOS)
> +    return NSStringFromClass([obj class]);
> +#else
>      return [obj className];
> +#endif

No idea why this is different.
Comment 5 Build Bot 2014-01-24 12:32:52 PST
Comment on attachment 222124 [details]
[iOS] Upstream changes in Tools/DumpRenderTree

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

New failing tests:
http/tests/misc/link-rel-icon-beforeload.html
Comment 6 Build Bot 2014-01-24 12:32:53 PST
Created attachment 222139 [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.5
Comment 7 Andy Estes 2014-01-24 14:10:33 PST
(In reply to comment #4)
> (From update of attachment 222124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222124&action=review
> 
> > Tools/DumpRenderTree/AccessibilityTextMarker.h:61
> > +#if PLATFORM(MAC) && SUPPORTS_AX_TEXTMARKERS
> 
> Don't think you need the PLATFORM(MAC) here any more.

Will change.

> 
> > Tools/DumpRenderTree/AccessibilityTextMarker.h:81
> > +#if PLATFORM(MAC) && SUPPORTS_AX_TEXTMARKERS
> 
> Ditto.
> 
> > Tools/DumpRenderTree/ios/Info.plist:12
> > +	<string>com.apple.DumpRenderTree</string>
> 
> org.webkit?

Sure.

> 
> > Tools/DumpRenderTree/ios/PerlSupport/IPhoneSimulatorNotification/Makefile.PL:1
> > +# Copyright (C) 2009 Apple Inc. All rights reserved.
> 
> 2014?

I think 2009 is probably correct. This code was in open source, then removed, then added back (albeit to a different directory) unchanged.

> 
> > Tools/DumpRenderTree/ios/PerlSupport/IPhoneSimulatorNotification/lib/IPhoneSimulatorNotification.pm:1
> > +# Copyright (C) 2009 Apple Inc. All rights reserved.
> 
> 2014?
> 
> > Tools/DumpRenderTree/ios/PerlSupport/Makefile:1
> > +# Copyright (C) 2009 Apple Inc. All rights reserved.
> 
> 2014
> 
> > Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:2
> > + * Copyright (C) 2005, 2006, 2007, 2009 Apple Inc. All rights reserved.
> 
> 2014
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:762
> > +    // See also -[TabDocument _updateTextSize] in MobileSafari/TabDocument.m
> 
> Remove this comment.

Ok.

> 
> > Tools/DumpRenderTree/mac/EventSendingController.mm:423
> > +#if !PLATFORM(IOS)
> >          flags |= NSControlKeyMask;
> > +#else
> > +        flags |= WebEventFlagMaskControl;
> > +#endif
> 
> These are ugly. Can we just make some common #ifdefs or an enum?

Probably. I'll look into it.

> 
> > Tools/DumpRenderTree/mac/ObjCPlugin.m:133
> > +#if PLATFORM(IOS)
> > +    return NSStringFromClass([obj class]);
> > +#else
> >      return [obj className];
> > +#endif
> 
> No idea why this is different.

Me neither :( Maybe we don't need it? I'll see.
Comment 8 Andy Estes 2014-01-26 16:18:50 PST
Committed r162817: <http://trac.webkit.org/changeset/162817>
Comment 9 Alexey Proskuryakov 2014-01-26 22:11:10 PST
This change broke http/tests/misc/link-rel-icon-beforeload.html, and EWS even told so. How could this patch get an r+ when it was known that it breaks a test?

This test is now broken on all Mac bots.
Comment 10 Alexey Proskuryakov 2014-01-26 22:20:52 PST
Hopefully fixed in <http://trac.webkit.org/r162826>.

I think that this patch could really benefit from another EWS round before landing - it obviously broke Mac, and it also was breaking EFL and Gtk builds. Were those fixed before landing?
Comment 11 Andy Estes 2014-01-26 23:32:33 PST
(In reply to comment #10)
> Hopefully fixed in <http://trac.webkit.org/r162826>.
> 
> I think that this patch could really benefit from another EWS round before landing - it obviously broke Mac, and it also was breaking EFL and Gtk builds. Were those fixed before landing?

Sorry Alexey. I was focusing on the build errors and overlooked the failing test. I did fix the build errors before landing.