Bug 64587 - Add support for retrieving an element in TreeScope by id to window.internals object.
Summary: Add support for retrieving an element in TreeScope by id to window.internals ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-15 01:02 PDT by Hayato Ito
Modified: 2011-08-01 05:26 PDT (History)
7 users (show)

See Also:


Attachments
first try to know which symbols we should export. (7.75 KB, patch)
2011-07-15 02:27 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
sync to ToT (8.02 KB, patch)
2011-07-18 23:27 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
2nd try. Add symbols (10.91 KB, patch)
2011-07-19 02:11 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
ready for review (12.73 KB, patch)
2011-07-19 03:51 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
revised (13.19 KB, patch)
2011-07-20 01:49 PDT, Hayato Ito
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-07-15 01:02:19 PDT
It would be nice that we can get an element in nested TreeScope by specifying 'id' and 'owner element'. That enables us to write the following code in tests:

  function getElementInShadow(path)
  {
    var ids = path.split('/');
    var element = document.getElementById(ids[0]);
    for (var i = 1; i < ids.length; ++i) {
        element = window.internals.getElementByIdInShadowHost(element, ids[i]);
    }
    return element;
  }

  var inputA = getElementInShadow('shadowHost/shadowHost2/inputA');
Comment 1 Hayato Ito 2011-07-15 01:09:11 PDT
I am thinking that we should have

  window.internals.getElementByIdInTreeScope(shadowRoot, ids[i])

instead of

  window.internals.getElementByIdInShadowHost(shadowHost, ids[i])
.
Comment 2 Hayato Ito 2011-07-15 02:27:29 PDT
Created attachment 100950 [details]
first try to know which symbols we should export.
Comment 3 Hayato Ito 2011-07-18 23:27:11 PDT
Created attachment 101273 [details]
sync to ToT
Comment 4 Collabora GTK+ EWS bot 2011-07-19 01:43:00 PDT
Comment on attachment 101273 [details]
sync to ToT

Attachment 101273 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9158050
Comment 5 Hayato Ito 2011-07-19 02:11:24 PDT
Created attachment 101289 [details]
2nd try. Add symbols
Comment 6 Hayato Ito 2011-07-19 03:51:28 PDT
Created attachment 101297 [details]
ready for review
Comment 7 Hajime Morrita 2011-07-19 04:06:14 PDT
Comment on attachment 101297 [details]
ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=101297&action=review

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:11
> +

Please make it clear that this test need internals object and doesn't work interactively.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:17
> +        element.setAttribute(name, attributes[name]);

Don't need braces if the block has single statement.
JS in WebKit basically follows our C++ style.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:60
> +

These helper functions look useful. How about to make it separate js file?

> Source/WebCore/testing/Internals.cpp:76
> +        ec = INVALID_ACCESS_ERR;

If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().

> Source/WebCore/testing/Internals.cpp:79
> +    return static_cast<ShadowRoot*>(treeScope)->getElementById(id);

I guess we have toShadowRoot().
Comment 8 Dominic Cooney 2011-07-19 07:24:06 PDT
Comment on attachment 101297 [details]
ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=101297&action=review

>> Source/WebCore/testing/Internals.cpp:76
>> +        ec = INVALID_ACCESS_ERR;
> 
> If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().

What about getElementByIdIn*Shadow*Root?

Tree root sounds like something else—the document, subtree root, whatever.
Comment 9 Hayato Ito 2011-07-20 01:47:32 PDT
Comment on attachment 101297 [details]
ready for review

Thank you for the review. I'll post a updated patch later.

View in context: https://bugs.webkit.org/attachment.cgi?id=101297&action=review

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:11
>> +
> 
> Please make it clear that this test need internals object and doesn't work interactively.

Done.

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:17
>> +        element.setAttribute(name, attributes[name]);
> 
> Don't need braces if the block has single statement.
> JS in WebKit basically follows our C++ style.

Done.

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:60
>> +
> 
> These helper functions look useful. How about to make it separate js file?

Done. I've moved createDom and createShadow functions into a separate js file as 'resources/create-dom.js'.
I might move getElemetnInShadow also when we have another layout test which uses this function.

>>> Source/WebCore/testing/Internals.cpp:76
>>> +        ec = INVALID_ACCESS_ERR;
>> 
>> If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().
> 
> What about getElementByIdIn*Shadow*Root?
> 
> Tree root sounds like something else—the document, subtree root, whatever.

Okay. I chose 'getElementByIdInShadowRoot'.

>> Source/WebCore/testing/Internals.cpp:79
>> +    return static_cast<ShadowRoot*>(treeScope)->getElementById(id);
> 
> I guess we have toShadowRoot().

Oh. When I wrote this patch, there was no header files which defines toShadowRoot. Now we have that in ShadowRoot.h.
Done.
Comment 10 Hayato Ito 2011-07-20 01:49:35 PDT
Created attachment 101440 [details]
revised
Comment 11 Hayato Ito 2011-07-31 19:16:34 PDT
ping? Could someone review this?
Comment 12 Hayato Ito 2011-08-01 05:26:18 PDT
Committed r92124: <http://trac.webkit.org/changeset/92124>