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.
Created attachment 137585 [details] Hopefully a patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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().
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?
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
(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.
Relevant chromium bug: http://code.google.com/p/chromium/issues/detail?id=123094
Comment on attachment 137595 [details] Patch Attachment 137595 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12423513
+dmazzoni, keeper of chrome a11y
Created attachment 137626 [details] Patch Moar build-webkit --ews.
Comment on attachment 137626 [details] Patch Attachment 137626 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12426032
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.
Created attachment 137789 [details] Patch Fourth time's the charm... .......... .
(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. :/
(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);
It'd have to be a pair of name/value tuples
(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 :)
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?
Comment on attachment 137789 [details] Patch Clearing flags on attachment: 137789 Committed r114686: <http://trac.webkit.org/changeset/114686>
All reviewed patches have been landed. Closing bug.