The generated names look like "<!--framePath //<!--frame0-->-->". Other browsers just return the original empty name.
Created attachment 5900 [details] test case
Created attachment 71356 [details] Patch with updated test case (Work-in-progress) Work-in-progress patch.
Comment on attachment 71356 [details] Patch with updated test case (Work-in-progress) I don't really understand the difference between name and domName. What use is a name if not for the DOM?
Created attachment 71377 [details] Patch with updated test case (Work-in-progress) Work-in-progress patch. Re-purpose FrameTree::name() to return the actual name of the frame as per the HTML5 spec. Added method FrameTree::uniqueName() to return a unique frame name (may be generated).
Created attachment 71666 [details] Patch with updated test case Updated change log. For the the port-specific API changes in this patch, I chose to preserve the current behavior of returning the unique frame name. We may want to consider changing the API behavior so that it returns the actual name of the frame, which may be the empty string. One concern I had was regressing the fix for bug 12732, which was landed in changeset 24396 <http://trac.webkit.org/changeset/24396>. The test case landed in this changeset <http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/iframe-set-inner-html.html> used frame name uniqueness for two unnamed frames to verify the fix. Clearly, this approach conflicts with this patch and the HTML5 spec with regards to frame names. Further investigation is needed to be able to devise an alternative test for bug 12732. Alternatively, we could expose a DRT method that returns the internal unique name for a frame and modify the test case to use this method. Does anyone have any additional insight/suggestions into bug 12732 and its test case?
Comment on attachment 71666 [details] Patch with updated test case View in context: https://bugs.webkit.org/attachment.cgi?id=71666&action=review > WebCore/page/FrameTree.cpp:45 > + m_name = name; > if (!parent()) { > - m_name = name; > + m_uniqueName = name; Is m_uniqueName guaranteed to be unique? If I explicitly set the name of two frames, can I cause a collision?
(In reply to comment #6) > (From update of attachment 71666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71666&action=review > > > WebCore/page/FrameTree.cpp:45 > > + m_name = name; > > if (!parent()) { > > - m_name = name; > > + m_uniqueName = name; > > Is m_uniqueName guaranteed to be unique? If I explicitly set the name of two frames, can I cause a collision? Yes, m_uniqueName is guaranteed to be unique by the if-condition in FrameTree::uniqueName() <http://trac.webkit.org/browser/trunk/WebCore/page/FrameTree.cpp?rev=60287#L104> and FrameTree::child(const AtomicString& name) <http://trac.webkit.org/browser/trunk/WebCore/page/FrameTree.cpp?rev=60287#L159>.
Comment on attachment 71666 [details] Patch with updated test case Great. Thanks for writing this patch.
Committed r70971: <http://trac.webkit.org/changeset/70971>
(In reply to comment #9) > Committed r70971: <http://trac.webkit.org/changeset/70971> This caused layout test failures. Rolled out patch in changeset 70976 <http://trac.webkit.org/changeset/70976> while I look into this further.
Regarding "We should consider exposing a DRT method for obtaining the internal frame name (which may be generated) and re-implementing this test to use it.", there's already layoutTestController.dumpChildFramesAsText() which exposes internal names.
Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID().
(In reply to comment #12) > Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID(). Generally, we try to name things with their DOM names, so we'll want this method to be called name(). However, landing a patch using domName() and then changing it to name() might be a good way of proving that we're correctly changed all the call sites.
(In reply to comment #13) > (In reply to comment #12) > > Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID(). > > Generally, we try to name things with their DOM names, so we'll want this method to be called name(). However, landing a patch using domName() and then changing it to name() might be a good way of proving that we're correctly changed all the call sites. With the passing of bug #48806, I plan to retry landing this patch in at least two parts based on Adam's suggestion later tonight. Also, based on Mihai Parparita's feedback I will also land an updated iframe-set-inner-html.html test that uses layoutTestController.dumpChildFramesAsText().
Split the patch into two parts: 1) substitute FrameTree::uniqueName() for FrameTree::name() and 2) Re-purpose FrameTree::name() for the DOM-specific name and implement support for the empty string as a valid frame name. Committed (1) in changeset 71216 <trac.webkit.org/changeset/71216> and changeset 71217 <http://trac.webkit.org/changeset/71217>. Committed (2) in changeset 71219 <http://trac.webkit.org/changeset/71219>.
http://trac.webkit.org/changeset/71219 might have broken GTK Linux 64-bit Debug The following tests are not passing: svg/css/getComputedStyle-basic.xhtml
(In reply to comment #16) > http://trac.webkit.org/changeset/71219 might have broken GTK Linux 64-bit Debug > The following tests are not passing: > svg/css/getComputedStyle-basic.xhtml This failure is not related to this bug. Using the build bot console view <http://build.webkit.org/console> this test started failing after the patch for bug #48608 <https://bugs.webkit.org/show_bug.cgi?id=48608> was landed in changeset 71218 <http://trac.webkit.org/changeset/71218>.