Closed
Bug 1061469
Opened 10 years ago
Closed 10 years ago
Add more test coverage for HTML Imports
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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 | ||
Updated•10 years ago
|
Assignee: nobody → gkrizsanits
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=95bb95ff7697
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8492986 -
Flags: review?(mrbkap)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8492988 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8492989 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8492990 -
Flags: review?(mrbkap)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8492992 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8492993 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8492994 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8492995 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8492996 -
Flags: review?(mrbkap)
Assignee | ||
Updated•10 years ago
|
Attachment #8492989 -
Attachment is patch: true
Assignee | ||
Updated•10 years ago
|
Blocks: webcomponents
Assignee | ||
Comment 11•10 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•10 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...
Updated•10 years ago
|
Attachment #8492986 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8492988 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8492989 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8492990 -
Flags: review?(mrbkap) → review+
Comment 13•10 years ago
|
||
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•10 years ago
|
Attachment #8492993 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8492994 -
Flags: review?(mrbkap) → review+
Comment 14•10 years ago
|
||
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•10 years ago
|
Attachment #8492996 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 15•10 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
Comment 16•10 years ago
|
||
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•10 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•10 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•10 years ago
|
Keywords: leave-open
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 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•10 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)
Updated•10 years ago
|
Attachment #8513397 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 22•10 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
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/34781adab80a https://hg.mozilla.org/mozilla-central/rev/f754a82c5560
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 24•10 years ago
|
||
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.
Description
•