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.
Created attachment 220710 [details] Patch proposal Here comes the patch.
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
(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
(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
Created attachment 220831 [details] Patch proposal New patch addressing Chris's comments
Comment on attachment 220831 [details] Patch proposal Thanks for the review
Comment on attachment 220831 [details] Patch proposal Clearing flags on attachment: 220831 Committed r161666: <http://trac.webkit.org/changeset/161666>
All reviewed patches have been landed. Closing bug.