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
|
||
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
•