Bug 84183 – [chromium] Add simplified API for iterating over a WebElement's attributes.

• Component: WebKit API
• Status: RESOLVED FIXED

[Andreas Kling]

To be able to freely refactor WebCore::Attribute, we first need to get rid of chromium's WebAttribute API.
This bug is about introducing a replacement API so that the chromium call sites for WebAttribute can be converted without breaking anything in the meantime.

[Andreas Kling]

Created attachment 137585 [details]
Hopefully a patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Andreas Kling]

Created attachment 137595 [details]
Patch

Actually Element::attributeCount() assumes that there are attributes present on the element, so we can't use that before checking hasAttributes().

[James Robinson]

I can't find a single bit of code using the Chromium WebKit API that uses the WebAttribute stuff.  I'm not sure if we had anything in the past.  In general, it's very hard to design good API without a specific use case in mind so I'd suggest not replacing WebAttribute with anything until we have one.

Darin?

[James Robinson]

Oh whoops, my searching skills suck.  Never mind, webkit/glue/webaccessibility.cc in the chromium tree uses it (although it's the only one).  This API would work fine for that, it appears

[Andreas Kling]

(In reply to comment #5)
> Oh whoops, my searching skills suck.  Never mind, webkit/glue/webaccessibility.cc in the chromium tree uses it (although it's the only one).  This API would work fine for that, it appears

Right. I should have mentioned that this is the (only?) call site I could find.

[Andreas Kling]

Relevant chromium bug: http://code.google.com/p/chromium/issues/detail?id=123094

[WebKit Review Bot]

Comment on attachment 137595 [details]
Patch

Attachment 137595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12423513

[Darin Fisher (:fishd, Google)]

+dmazzoni, keeper of chrome a11y

[Andreas Kling]

Created attachment 137626 [details]
Patch

Moar build-webkit --ews.

[WebKit Review Bot]

Comment on attachment 137626 [details]
Patch

Attachment 137626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12426032

[Dominic Mazzoni]

Thanks! This new attribute interface looks great to me.

In the future I was hoping to optimize webaccessibility.cc by only fetching the attributes we actually need. Would it be possible to have an efficient getAttributeByName method?

What about a getAttributesByName (plural) where I could pass a list of all of the attributes I'm interested in, and get a list of all matches? The list would be the same for every element (id, class, role, and then ~20 aria attributes from aria-atomic to aria-valuenow) so it'd be nice to figure out the most efficient interface to do this since we need to retrieve these attributes for every node of the tree.

You certainly don't have to add anything else now - this patch is great - but I'd like to know your thoughts on what the best interface might look like.

[Andreas Kling]

Created attachment 137789 [details]
Patch

Fourth time's the charm...   ..........   .

[Andreas Kling]

(In reply to comment #12)
> Thanks! This new attribute interface looks great to me.
> 
> In the future I was hoping to optimize webaccessibility.cc by only fetching the attributes we actually need. Would it be possible to have an efficient getAttributeByName method?
> 
> What about a getAttributesByName (plural) where I could pass a list of all of the attributes I'm interested in, and get a list of all matches? The list would be the same for every element (id, class, role, and then ~20 aria attributes from aria-atomic to aria-valuenow) so it'd be nice to figure out the most efficient interface to do this since we need to retrieve these attributes for every node of the tree.
> 
> You certainly don't have to add anything else now - this patch is great - but I'd like to know your thoughts on what the best interface might look like.

Given how attributes are stored (unsorted vector), there's not much we can do, as you always have to iterate over the vector to get all the attributes. :/

[Dimitri Glazkov (Google)]

(In reply to comment #14)
> (In reply to comment #12)
> > Thanks! This new attribute interface looks great to me.
> > 
> > In the future I was hoping to optimize webaccessibility.cc by only fetching the attributes we actually need. Would it be possible to have an efficient getAttributeByName method?
> > 
> > What about a getAttributesByName (plural) where I could pass a list of all of the attributes I'm interested in, and get a list of all matches? The list would be the same for every element (id, class, role, and then ~20 aria attributes from aria-atomic to aria-valuenow) so it'd be nice to figure out the most efficient interface to do this since we need to retrieve these attributes for every node of the tree.
> > 
> > You certainly don't have to add anything else now - this patch is great - but I'd like to know your thoughts on what the best interface might look like.
> 
> Given how attributes are stored (unsorted vector), there's not much we can do, as you always have to iterate over the vector to get all the attributes. :/

Can this just populate a vector of strings? This seems like the simplest thing to do:

like void getAttributes(WebVector<WebCString>& attributes);

[James Robinson]

It'd have to be a pair of name/value tuples

[Andreas Kling]

(In reply to comment #16)
> It'd have to be a pair of name/value tuples

Is that an "r-, please replace this by a function that returns a list of name/value pairs"? I'm blocked on this relatively tiny change, so it'd be neat if someone could make a decision :)

[Dominic Mazzoni]

Getting a vector of pairs of strings seems worse to me - you'd have to pay the cost of allocating the whole vector of names and values, even if you only wanted to grab the value of one particular attribute.

I like the current patch. Dimitri?

[Andreas Kling]

Comment on attachment 137789 [details]
Patch

Clearing flags on attachment: 137789

Committed r114686: <http://trac.webkit.org/changeset/114686>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.