Add more test coverage for HTML Imports

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

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

5 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

5 years ago
Assignee: nobody → gkrizsanits
(Assignee)

Updated

5 years ago
Depends on: 877072
(Assignee)

Comment 2

5 years ago
Attachment #8492986 - Flags: review?(mrbkap)
(Assignee)

Comment 4

5 years ago
Attachment #8492989 - Flags: review?(mrbkap)
(Assignee)

Comment 5

5 years ago
Attachment #8492990 - Flags: review?(mrbkap)
(Assignee)

Comment 6

5 years ago
Attachment #8492992 - Flags: review?(mrbkap)
(Assignee)

Comment 7

5 years ago
Attachment #8492993 - Flags: review?(mrbkap)
(Assignee)

Comment 8

5 years ago
Attachment #8492994 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
Attachment #8492995 - Flags: review?(mrbkap)
(Assignee)

Updated

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

Updated

5 years ago
Blocks: 811542
(Assignee)

Comment 11

5 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

5 years ago
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...
Attachment #8492986 - Flags: review?(mrbkap) → review+
Attachment #8492988 - Flags: review?(mrbkap) → review+
Attachment #8492989 - Flags: review?(mrbkap) → review+
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+
Attachment #8492993 - Flags: review?(mrbkap) → review+
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+
Attachment #8492996 - Flags: review?(mrbkap) → review+
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

5 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)

Updated

5 years ago
Keywords: leave-open
Depends on: 1079123
Depends on: 1083836
Depends on: 1079167
(Assignee)

Comment 20

5 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

5 years ago
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)
Attachment #8513397 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/34781adab80a
https://hg.mozilla.org/mozilla-central/rev/f754a82c5560
Status: NEW → RESOLVED
Last Resolved: 5 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.