Closed Bug 1061469 Opened 10 years ago Closed 10 years ago

Add more test coverage for HTML Imports

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

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
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: nobody → gkrizsanits
Depends on: 877072
Attachment #8492986 - Flags: review?(mrbkap)
Attachment #8492989 - Flags: review?(mrbkap)
Attachment #8492990 - Flags: review?(mrbkap)
Attachment #8492992 - Flags: review?(mrbkap)
Attachment #8492993 - Flags: review?(mrbkap)
Attachment #8492994 - Flags: review?(mrbkap)
Attachment #8492995 - Flags: review?(mrbkap)
Attachment #8492989 - Attachment is patch: true
(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.
Attached patch test generatorSplinter Review
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
(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?
Keywords: leave-open
Depends on: 1079123
Depends on: 1083836
Depends on: 1079167
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.
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+
Status: NEW → RESOLVED
Closed: 10 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.

Attachment

General

Created:
Updated:
Size: