Bug 84183 - [chromium] Add simplified API for iterating over a WebElement's attributes.
Summary: [chromium] Add simplified API for iterating over a WebElement's attributes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks: 83440
  Show dependency treegraph
 
Reported: 2012-04-17 12:49 PDT by Andreas Kling
Modified: 2012-04-19 16:00 PDT (History)
7 users (show)

See Also:


Attachments
Hopefully a patch (2.71 KB, patch)
2012-04-17 12:50 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (2.74 KB, patch)
2012-04-17 13:43 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (2.79 KB, patch)
2012-04-17 16:04 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (2.81 KB, patch)
2012-04-18 15:37 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-04-17 12:49:42 PDT
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.
Comment 1 Andreas Kling 2012-04-17 12:50:58 PDT
Created attachment 137585 [details]
Hopefully a patch
Comment 2 WebKit Review Bot 2012-04-17 12:54:02 PDT
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.
Comment 3 Andreas Kling 2012-04-17 13:43:07 PDT
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().
Comment 4 James Robinson 2012-04-17 14:21:37 PDT
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?
Comment 5 James Robinson 2012-04-17 14:24:11 PDT
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
Comment 6 Andreas Kling 2012-04-17 14:25:27 PDT
(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.
Comment 7 Andreas Kling 2012-04-17 15:15:02 PDT
Relevant chromium bug: http://code.google.com/p/chromium/issues/detail?id=123094
Comment 8 WebKit Review Bot 2012-04-17 15:30:16 PDT
Comment on attachment 137595 [details]
Patch

Attachment 137595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12423513
Comment 9 Darin Fisher (:fishd, Google) 2012-04-17 15:56:56 PDT
+dmazzoni, keeper of chrome a11y
Comment 10 Andreas Kling 2012-04-17 16:04:56 PDT
Created attachment 137626 [details]
Patch

Moar build-webkit --ews.
Comment 11 WebKit Review Bot 2012-04-17 17:03:55 PDT
Comment on attachment 137626 [details]
Patch

Attachment 137626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12426032
Comment 12 Dominic Mazzoni 2012-04-17 23:10:37 PDT
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.
Comment 13 Andreas Kling 2012-04-18 15:37:20 PDT
Created attachment 137789 [details]
Patch

Fourth time's the charm...   ..........   .
Comment 14 Andreas Kling 2012-04-18 15:38:42 PDT
(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. :/
Comment 15 Dimitri Glazkov (Google) 2012-04-19 11:08:08 PDT
(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);
Comment 16 James Robinson 2012-04-19 11:21:03 PDT
It'd have to be a pair of name/value tuples
Comment 17 Andreas Kling 2012-04-19 14:09:22 PDT
(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 :)
Comment 18 Dominic Mazzoni 2012-04-19 15:11:02 PDT
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 19 Andreas Kling 2012-04-19 16:00:12 PDT
Comment on attachment 137789 [details]
Patch

Clearing flags on attachment: 137789

Committed r114686: <http://trac.webkit.org/changeset/114686>
Comment 20 Andreas Kling 2012-04-19 16:00:19 PDT
All reviewed patches have been landed.  Closing bug.