Add more test coverage for HTML Imports

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

3.51 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.87 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.82 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.55 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.16 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.53 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.72 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.24 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.01 KB, patch
Details | Diff | Splinter Review
4.93 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
HTML Imports are under tested. I'm going to add more tests and since that process will likely help finding bugs and missing features, this bug will probably serve as a meta bug.
(Assignee)

Updated

3 years ago
Assignee: nobody → gkrizsanits
(Assignee)

Comment 1

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=95bb95ff7697
(Assignee)

Updated

3 years ago
Depends on: 877072
(Assignee)

Comment 2

3 years ago
Created attachment 8492986 [details] [diff] [review]
Part 1: Cyclic test for Imports 1
Attachment #8492986 - Flags: review?(mrbkap)
(Assignee)

Comment 3

3 years ago
Created attachment 8492988 [details] [diff] [review]
Part2: ImportManager::AddLoaderWithNewURI should only return main referrersAddLoaderWithNewURI fix
Attachment #8492988 - Flags: review?(mrbkap)
(Assignee)

Comment 4

3 years ago
Created attachment 8492989 [details] [diff] [review]
Part 3: Cyclic test for Imports 2
Attachment #8492989 - Flags: review?(mrbkap)
(Assignee)

Comment 5

3 years ago
Created attachment 8492990 [details] [diff] [review]
part4: Cyclic test for Imports 3
Attachment #8492990 - Flags: review?(mrbkap)
(Assignee)

Comment 6

3 years ago
Created attachment 8492992 [details] [diff] [review]
part 5: Fixing up update for Imports
Attachment #8492992 - Flags: review?(mrbkap)
(Assignee)

Comment 7

3 years ago
Created attachment 8492993 [details] [diff] [review]
part 6: Cyclic test for Imports 4
Attachment #8492993 - Flags: review?(mrbkap)
(Assignee)

Comment 8

3 years ago
Created attachment 8492994 [details] [diff] [review]
part7: Cyclic test for Imports 5
Attachment #8492994 - Flags: review?(mrbkap)
(Assignee)

Comment 9

3 years ago
Created attachment 8492995 [details] [diff] [review]
part 8: Encoding for Imports
Attachment #8492995 - Flags: review?(mrbkap)
(Assignee)

Comment 10

3 years ago
Created attachment 8492996 [details] [diff] [review]
part 9: defaultView should be null for imports
Attachment #8492996 - Flags: review?(mrbkap)
(Assignee)

Updated

3 years ago
Attachment #8492989 - Attachment is patch: true
(Assignee)

Updated

3 years ago
Blocks: 811542
(Assignee)

Comment 11

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> Created attachment 8492988 [details] [diff] [review]
> Part2: ImportManager::AddLoaderWithNewURI should only return main
> referrersAddLoaderWithNewURI fix

Test 2 helped me realize that when I want find the blocking predecessor with GetNearestPredecessor, that algorithm should only ever care about primaly links, since those are the only ones that matter in the execution order (and also since we want to avoid cycles...).

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> Created attachment 8492992 [details] [diff] [review]
> part 5: Fixing up update for Imports

Test 4 helped finding another bug, that we need to update a node not only when the primary link changed, but also if the structure of the spanning tree (tree of primary links) has changed in
such a way that GetNearestPredecessor would return something else than it used to.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> Created attachment 8492995 [details] [diff] [review]
> part 8: Encoding for Imports

This is just a detail in the spec that I forgot to take care of sooner.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> Created attachment 8492996 [details] [diff] [review]
> part 9: defaultView should be null for imports

There were several debates about what defaultView should be, it seems like this is the final
version.
(Assignee)

Comment 12

3 years ago
Created attachment 8493869 [details] [diff] [review]
test generator

Most of these tests are generated. The generator test is kind of boring, but I attach it since it might be easier to review the tests. The arrays at the beginning shows the structure of the tests.

links['B'] = ['C', 'A']; for example means import B have a link import to import C and import A in this order. I hope it helps...

Updated

3 years ago
Attachment #8492986 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492988 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492989 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492990 - Flags: review?(mrbkap) → review+
Comment on attachment 8492992 [details] [diff] [review]
part 5: Fixing up update for Imports

Review of attachment 8492992 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ImportManager.cpp
@@ +90,5 @@
>  
>  bool
>  ImportLoader::Updater::ShouldUpdate(nsTArray<nsINode*>& aNewPath)
>  {
> +  if (mLoader->Manager()->GetNearestPredecessor(mLoader->GetMainReferrer()) != 

Uber-nit: trailing whitespace.
Attachment #8492992 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492993 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492994 - Flags: review?(mrbkap) → review+
Comment on attachment 8492995 [details] [diff] [review]
part 8: Encoding for Imports

Review of attachment 8492995 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +3432,5 @@
>    // XXX it would be a good idea to assert the sanity of the argument,
>    // but before we figure out what to do about non-Encoding Standard
>    // encodings in the charset menu and in mailnews, assertions are futile.
>    if (!mCharacterSet.Equals(aCharSetID)) {
> +    if (mMasterDocument && !aCharSetID.Equals(NS_LITERAL_CSTRING("UTF-8"))) {

aCharSetID.EqualsLiteral(...)
Attachment #8492995 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Attachment #8492996 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 15

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c3be44c703
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17c54acc6645
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27f8d501cef5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1008e478d100
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01ead7a7bb5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62486f51f316
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c44a02925cb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3db413f9da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8378f26b0730
sorry had to backout this changes for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2706700&repo=mozilla-inbound in Android
(Assignee)

Comment 17

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #16)
> sorry had to backout this changes for test failures like
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=2706700&repo=mozilla-inbound in Android

These failures seems to be unrelated to my patches. And last time I tested these patches on try I have not seen issues like these. Are you sure they caused these failures?
(Assignee)

Comment 18

3 years ago
Not sure what happened but the last patch seems to cause the failure. Maybe because I should bump the iid on nsIDocument... Until I figure it out I push the ones that were green:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a94b3f6394
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8162e9e76065
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c9780b72878
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d90c3624203
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd96e70b321c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da69c023cce9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cda7419a220b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/52b510bad6a7
(Assignee)

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e2a94b3f6394
https://hg.mozilla.org/mozilla-central/rev/8162e9e76065
https://hg.mozilla.org/mozilla-central/rev/3c9780b72878
https://hg.mozilla.org/mozilla-central/rev/1d90c3624203
https://hg.mozilla.org/mozilla-central/rev/cd96e70b321c
https://hg.mozilla.org/mozilla-central/rev/da69c023cce9
https://hg.mozilla.org/mozilla-central/rev/cda7419a220b
https://hg.mozilla.org/mozilla-central/rev/52b510bad6a7
Flags: in-testsuite+

Updated

3 years ago
Depends on: 1079123

Updated

3 years ago
Depends on: 1083836

Updated

3 years ago
Depends on: 1079167
(Assignee)

Comment 20

3 years ago
The defaultView being always null patch is not needed any longer, only the test from it, and until I figure out what is going on I will back out test_cycle_5.
(Assignee)

Comment 21

3 years ago
Created attachment 8513397 [details] [diff] [review]
part 10: Removing intermittent import test.

The intermittent timeout results on inbound does not help me understanding the underlying issue, so I think we should just remove it until I figure out what is going on, so it won't cause frustration for others.
Attachment #8513397 - Flags: review?(mrbkap)

Updated

3 years ago
Attachment #8513397 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 22

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34781adab80a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f754a82c5560
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/34781adab80a
https://hg.mozilla.org/mozilla-central/rev/f754a82c5560
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I backported the test removal patch to Aurora as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/71ac3e837bb7
You need to log in before you can comment on or make changes to this bug.