Bug 201536 - Test SVGViewSpec behavior across page-loads with different anchors
Summary: Test SVGViewSpec behavior across page-loads with different anchors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-06 00:55 PDT by Nikolas Zimmermann
Modified: 2019-09-17 12:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.09 KB, patch)
2019-09-06 00:55 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (12.86 KB, patch)
2019-09-09 13:55 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (12.85 KB, patch)
2019-09-10 03:00 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (12.79 KB, patch)
2019-09-10 11:29 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2019-09-06 00:55:06 PDT
Split off a layout test from bug 94469, that is unrelated to SMIL animations of the SVGViewElement, but helpful to document the expected SVGViewElement/SVGViewSpec behavior.
Comment 1 Nikolas Zimmermann 2019-09-06 00:55:46 PDT
Created attachment 378165 [details]
Patch
Comment 2 Nikolas Zimmermann 2019-09-06 06:33:11 PDT
The failure in the ios-wk2 EWS bot is unrelated to the patch.
Comment 3 Rob Buis 2019-09-09 04:20:23 PDT
Comment on attachment 378165 [details]
Patch

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

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:23
> +    document.body.appendChild(script);

Why is the js-test-post.js script added dynamically at the end? Could it not be declared right after js-test-pre.js?

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173
> +iframeElement.setAttribute("style", "display: block");

Is this needed?
Comment 4 Nikolas Zimmermann 2019-09-09 05:13:01 PDT
(In reply to Rob Buis from comment #3)
Thanks Rob!
 
> > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:23
> > +    document.body.appendChild(script);
> 
> Why is the js-test-post.js script added dynamically at the end? Could it not
> be declared right after js-test-pre.js?
Indeed, there is a 'jsTestIsAsync' mode that I can use.
But still it cannot be loaded right after js-test-pre.js, because I need a chance to say "jsTestIsAsync = true", which is typically done after the description() call, when comparing to other tests.

But I should be able to get rid of the custom completeJSTest() method: avoiding boilerplate is nice, so I will try that.

> 
> > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173
> > +iframeElement.setAttribute("style", "display: block");
> 
> Is this needed?

I will see how it looks in MiniBrowser w/o that. For the actual test result, it makes no difference.
Comment 5 Rob Buis 2019-09-09 05:34:58 PDT
> > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173
> > > +iframeElement.setAttribute("style", "display: block");
> > 
> > Is this needed?
> 
> I will see how it looks in MiniBrowser w/o that. For the actual test result,
> it makes no difference.

From my testing display inline will be used, but indeed if it does not change the test it is better to remove unneeded changes.
Comment 6 Nikolas Zimmermann 2019-09-09 13:53:24 PDT
(In reply to Rob Buis from comment #5)
> > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173
> > > > +iframeElement.setAttribute("style", "display: block");
> > > 
> > > Is this needed?
> > 
> > I will see how it looks in MiniBrowser w/o that. For the actual test result,
> > it makes no difference.
> 
> From my testing display inline will be used, but indeed if it does not
> change the test it is better to remove unneeded changes.

Visually there is no difference, I removed the line it is not necessary.
Please check the new patch :-)
Comment 7 Nikolas Zimmermann 2019-09-09 13:55:34 PDT
Created attachment 378405 [details]
Patch
Comment 8 Rob Buis 2019-09-10 00:05:11 PDT
Comment on attachment 378405 [details]
Patch

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

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:14
> +    testRunner.waitUntilDone();

This should not be needed anymore.

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:73
> +    debug("Verify that no load was peformed, but only a different view was set on the same document");

performed.

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:118
> +    debug("Verify that no load was peformed, but only a different view was set on the same document");

Ditto.
Comment 9 Nikolas Zimmermann 2019-09-10 02:58:38 PDT
(In reply to Rob Buis from comment #8)
> Comment on attachment 378405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378405&action=review
> 
> > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:14
> > +    testRunner.waitUntilDone();
> 
> This should not be needed anymore.
Indeed, good catch. This framework has changed since I looked at it last time :-)

> 
> > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:73
> > +    debug("Verify that no load was peformed, but only a different view was set on the same document");
> 
> performed.
Done.
Comment 10 Nikolas Zimmermann 2019-09-10 03:00:23 PDT
Created attachment 378451 [details]
Patch
Comment 11 Rob Buis 2019-09-10 07:19:40 PDT
Comment on attachment 378451 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        navigating between anchors, which indicates that no new document was created. Verify that the SVGViewSpec API works as intended, across navigations.

Please try to stick to max 100 characters here.
Comment 12 Nikolas Zimmermann 2019-09-10 10:51:40 PDT
(In reply to Rob Buis from comment #11)
> Please try to stick to max 100 characters here.
Ok. Shall I upload a new patch? If not, how shall we land it? Will you set cq+?
Comment 13 Rob Buis 2019-09-10 11:19:05 PDT
(In reply to Nikolas Zimmermann from comment #12)
> (In reply to Rob Buis from comment #11)
> > Please try to stick to max 100 characters here.
> Ok. Shall I upload a new patch? If not, how shall we land it? Will you set
> cq+?

Sure, upload a new patch fixing the width and I will cq+.
Comment 14 Nikolas Zimmermann 2019-09-10 11:29:10 PDT
Created attachment 378470 [details]
Patch
Comment 15 Nikolas Zimmermann 2019-09-10 12:00:09 PDT
Thanks Rob! Have a good night.
Comment 16 WebKit Commit Bot 2019-09-10 12:57:28 PDT
Comment on attachment 378470 [details]
Patch

Clearing flags on attachment: 378470

Committed r249730: <https://trac.webkit.org/changeset/249730>
Comment 17 WebKit Commit Bot 2019-09-10 12:57:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-09-10 12:58:18 PDT
<rdar://problem/55232996>
Comment 19 Radar WebKit Bug Importer 2019-09-10 12:58:18 PDT
<rdar://problem/55232995>
Comment 20 Nikolas Zimmermann 2019-09-10 13:26:10 PDT
Oops, the title of the bug in the ChangeLog is wrong, it should read "Test SVGViewSpec behavior across page-loads with different anchors" instead of "SMIL animations of SVG <view> element have no effect".
Comment 21 Nikolas Zimmermann 2019-09-17 12:36:00 PDT
(In reply to Nikolas Zimmermann from comment #20)
> Oops, the title of the bug in the ChangeLog is wrong, it should read "Test
> SVGViewSpec behavior across page-loads with different anchors" instead of
> "SMIL animations of SVG <view> element have no effect".

For the record: that was fixed already.