Bug 27956 - AX: roleValue should be cached for performance
Summary: AX: roleValue should be cached for performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-03 15:11 PDT by chris fleizach
Modified: 2009-08-06 18:57 PDT (History)
0 users

See Also:


Attachments
patch (18.97 KB, patch)
2009-08-03 15:32 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (9.12 KB, patch)
2009-08-03 16:43 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (9.64 KB, patch)
2009-08-04 10:14 PDT, chris fleizach
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2009-08-03 15:11:42 PDT
a lot of calls to roleValue can be made in the course of an AX object's lifetime. i think it would be beneficial to cache that value
Comment 1 chris fleizach 2009-08-03 15:13:10 PDT
my initial testing shows a 5.1% speedup by caching the role value
Comment 2 chris fleizach 2009-08-03 15:32:06 PDT
testing was done by navigating a web page and totaling the time spent in copyAttributeValue. Of course roleValue is called in other places, so this speedup will be felt in other places as well
Comment 3 chris fleizach 2009-08-03 15:32:20 PDT
Created attachment 34009 [details]
patch
Comment 4 Mark Rowe (bdash) 2009-08-03 15:41:47 PDT
Comment on attachment 34009 [details]
patch

> Index: WebCore/accessibility/AccessibilityImageMapLink.cpp
> ===================================================================
> --- WebCore/accessibility/AccessibilityImageMapLink.cpp	(revision 46732)
> +++ WebCore/accessibility/AccessibilityImageMapLink.cpp	(working copy)
> @@ -43,9 +43,10 @@ namespace WebCore {
>  using namespace HTMLNames;
>  
>  AccessibilityImageMapLink::AccessibilityImageMapLink()
> -    : m_areaElement(0), 
> -      m_mapElement(0)
> +    : m_areaElement(0)
> +    , m_mapElement(0)
>  {
> +    m_role = WebCoreLinkRole;
>  }

Initialization of this form should happen in the initialization list rather than via assignment in the constructor body.
Comment 5 chris fleizach 2009-08-03 15:42:54 PDT
m_role is a protected variable in AccessibilityObject. When i tried to initialize it in the constructor list, it wouldn't compile
Comment 6 chris fleizach 2009-08-03 15:49:07 PDT
specifically
/Volumes/data/WebKit/WebCore/accessibility/AccessibilityImageMapLink.cpp:48: error: class ‘WebCore::AccessibilityImageMapLink’ does not have any field named ‘m_role’
Comment 7 chris fleizach 2009-08-03 16:09:06 PDT
Comment on attachment 34009 [details]
patch

obsoleting this patch
Comment 8 chris fleizach 2009-08-03 16:43:27 PDT
Created attachment 34018 [details]
patch
Comment 9 Eric Seidel (no email) 2009-08-04 08:59:45 PDT
Comment on attachment 34018 [details]
patch

setRoleValue would normally take an argument.  Maybe initializeRoleValues() or determineRoleValues()?   Do determineRoleValue() and determineAriaRole() need to be separate, or can they just be both part of a determinRoleValue()?  Is this something we could just do on first access to roleValue()?  or do we need to call this explicitly?
Comment 10 chris fleizach 2009-08-04 09:07:03 PDT
i think we can re-word some of these methods
Comment 11 chris fleizach 2009-08-04 10:07:07 PDT
unfortunately, we have to determine the role at initialization because too many methods rely on roleValue() to be const (so we can't set the role in roleValue() the first time someone asks for it)
Comment 12 chris fleizach 2009-08-04 10:14:02 PDT
Created attachment 34073 [details]
patch
Comment 13 chris fleizach 2009-08-04 10:14:35 PDT
changed set*Role to updateAccessibilityRole()
Comment 14 Eric Seidel (no email) 2009-08-06 18:15:36 PDT
Comment on attachment 34073 [details]
patch

Looks OK.
Comment 15 chris fleizach 2009-08-06 18:57:04 PDT
http://trac.webkit.org/changeset/46878