Bug 126689 – Move platformName property from testRunner to accessibilityController

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mario Sanchez Prada]

This platformName property was originally introduced to be able to write some parts of the layout tests in a platform-specific way, which might sound like a weird idea at a first glance but made sense in the context of accessibility, where different ports expect some times different accessibility hierarchies exposed, and having different test expectations didn't look like good enough (which usually lead to platform-specific tests, causing duplication).

Still, this property has been added in testRunner (former layoutTestController) for the sake of providing the functionality in a more generic way, in case it was useful for other tests. However, the truth is that after all this time only accessibility tests are using it, so I think it might be better to move it out of testRunner and into accessibilityController, where it seems to be a better fit.

Additionally, if we do this move we could refactor the concept itself of "platform name", which so far has been tied to the "port name" (e.g. "gtk", "mac", "efl"). Thus, my proposal is to move this into accessibilityController and also change the concept of "platform name" to mean "the name of the accessibility platform", that is, "atk" (for gtk and efl), "mac" and win (only for WK1, it seems).

Doing this change, the merge among platform/mac/accessibility/role-subrole-roledescription.html and platform/gtk/accessibility/roles-exposed.html tests would be cleaner and, in my opinion, simpler. Of course we would need to adapt the few layout tests that are now using testRunner.platformName, but that should not be a big deal.

I'll post a patch soon.

[Mario Sanchez Prada]

Created attachment 220710 [details]
Patch proposal

Here comes the patch.

[chris fleizach]

Comment on attachment 220710 [details]
Patch proposal

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

> Tools/ChangeLog:12
> +        gtk, efl) but between platofms (atk, mac, win).

platofrms -> platforms

> Tools/DumpRenderTree/AccessibilityController.cpp:161
> +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(WIN) || PLATFORM(EFL)

is there any platform that doesn't want this?
seems like we prob don't need the #if check

[Mario Sanchez Prada]

(In reply to comment #2)
> (From update of attachment 220710 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220710&action=review
> 
> > Tools/ChangeLog:12
> > +        gtk, efl) but between platofms (atk, mac, win).
> 
> platofrms -> platforms

Ok.

> > Tools/DumpRenderTree/AccessibilityController.cpp:161
> > +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(WIN) || PLATFORM(EFL)
> 
> is there any platform that doesn't want this?
> seems like we prob don't need the #if check

I thought the same, but then I though of the IOS version of AccessibilityController (all guarded with #if PLATFORM(IOS)) and it was not clear to me whether I should added there, and also what to report as the platform name ("mac"? "ios"?)

If you can clarify this, I'm happy to include the related bits in AccessibilityControllerIOS and get rid of all these checks here

[chris fleizach]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 220710 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220710&action=review
> > 
> > > Tools/ChangeLog:12
> > > +        gtk, efl) but between platofms (atk, mac, win).
> > 
> > platofrms -> platforms
> 
> Ok.
> 
> > > Tools/DumpRenderTree/AccessibilityController.cpp:161
> > > +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(WIN) || PLATFORM(EFL)
> > 
> > is there any platform that doesn't want this?
> > seems like we prob don't need the #if check
> 
> I thought the same, but then I though of the IOS version of AccessibilityController (all guarded with #if PLATFORM(IOS)) and it was not clear to me whether I should added there, and also what to report as the platform name ("mac"? "ios"?)
> 
> If you can clarify this, I'm happy to include the related bits in AccessibilityControllerIOS and get rid of all these checks here

Yes, let's add it to iOS as well

[Mario Sanchez Prada]

Created attachment 220831 [details]
Patch proposal

New patch addressing Chris's comments

[Mario Sanchez Prada]

Comment on attachment 220831 [details]
Patch proposal

Thanks for the review

[WebKit Commit Bot]

Comment on attachment 220831 [details]
Patch proposal

Clearing flags on attachment: 220831

Committed r161666: <http://trac.webkit.org/changeset/161666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.