Closed
Bug 660959
Opened 12 years ago
Closed 12 years ago
Figure out a better setup for figuring out link state
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: bzbarsky, Assigned: dzbarsky)
References
Details
Attachments
(1 file, 12 obsolete files)
34.57 KB,
patch
|
Details | Diff | Splinter Review |
It's expensive, so we do it lazily.... the problem is when. Right now we have hacks in style resolution to trigger it, basically, but that's not so hot if we want to parallelize. Can we do better?
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dzbarsky
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #541461 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 2•12 years ago
|
||
Comment on attachment 541461 [details] [diff] [review] Update link state before resolving style Does this not break querySelector? What about resolutions coming via ReResolveStyleContext? I'm pretty sure this change is wrong....
Attachment #541461 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #541461 -
Attachment is obsolete: true
Attachment #542144 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 4•12 years ago
|
||
Comment on attachment 542144 [details] [diff] [review] Update link state less lazily Hmm. I think this should work, but can you add some of the tests your initial patch would have failed, please? And fix the commit message!
Attachment #542144 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•12 years ago
|
||
This patch does have tests. Do you mean some other tests?
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
![]() |
Reporter | |
Updated•12 years ago
|
Keywords: checkin-needed
![]() |
Reporter | |
Comment 7•12 years ago
|
||
Comment on attachment 542339 [details] [diff] [review] Patch to check in r=bz Er, I somehow completely missed the tests, sorry. The first test is wrong. It needs to do $("content").querySelector, and ideally put an id on the <a> and make sure querySelector returned the right thing. So something like: <div id="content" style="display: none"> <a href="#" id="testa"></a> </div> <script> is($("content").querySelector(":link, :visited"), $("testa"), "Should find a link even in a display:none subtree"); </script> The second test is fine, though you can replace ".getPropertyValue('color').toString()" with ".color" there.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #542144 -
Attachment is obsolete: true
Attachment #542339 -
Attachment is obsolete: true
Attachment #542592 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 9•12 years ago
|
||
Comment on attachment 542592 [details] [diff] [review] Patch r=me
Attachment #542592 -
Flags: review?(bzbarsky) → review+
![]() |
Reporter | |
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•12 years ago
|
||
This one doesn't have a slowdown on the selector test
Attachment #542592 -
Attachment is obsolete: true
Attachment #543006 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
![]() |
Reporter | |
Comment 11•12 years ago
|
||
Comment on attachment 543006 [details] [diff] [review] Patch I just realized that the nsGlobalWindow change is wrong. You should QI to nsIContent, then check that it's non-null and IsElement() and then call AsElement().... r=me with that.
Attachment #543006 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #543006 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 13•12 years ago
|
||
Actually, I think there's still a problem here... Consider a display:none subtree containing this markup: <a href="http://www.example.com"></a> <div id="foo"> <span></span> </div> And a call to document.getElementById("foo").querySelector(":link + * span, :visited + * span"). This should return the span. I bet with your changes it doesn't....
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #543170 -
Attachment is obsolete: true
Attachment #545507 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 15•12 years ago
|
||
Two comments offhand: 1) Why not just use the hashtable as a member, instead of pointer-to-hashtable? This hashtable will be used on basically every page.... 2) You probably need to either have a scriptblocker in FlushPendingLinkUpdates or stick all the element pointers into an array before making the RequestLinkStateUpdate calls, because I don't quite trust those calls to not add/remove things in this hashtable. Scriptblocker is probably simpler. I'll look through the rest carefully tomorrow.
Assignee | ||
Comment 16•12 years ago
|
||
Dromaeo DOM results Before patch: appendChild: 653.94runs/s ±0.89% insertBefore: 442.44runs/s ±1.46% After patch: appendChild: 643.07runs/s ±0.99% insertBefore: 437.16runs/s ±2.30%
Attachment #545507 -
Attachment is obsolete: true
Attachment #545507 -
Flags: review?(bzbarsky)
Attachment #545959 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 17•12 years ago
|
||
Comment on attachment 545959 [details] [diff] [review] Patch Please document your new methods and member. I don't understand the block at the beginning of UnregisterPendingLinkUpdate. Why is it there? In UnbindFromTree, you can set doc to GetCurrentDoc so you only unregister when you might possibly be registered. To address the Element precondition thinkg you added, do you want to just switch the document hashtable to using Link instead of Element? That seems like it would be cleaner... r=me with those changes. Thank you!
Attachment #545959 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 18•12 years ago
|
||
> I don't understand the block at the beginning of UnregisterPendingLinkUpdate. > Why is it there?
I removed the Get call. There's no point in performing the hash lookup if there are no elements in the table.
Attachment #545959 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 19•12 years ago
|
||
That last patch still needs comments. It's also missing the changes to Link.h and Link.cpp, and the nsDocument code is still trying to call RequestLinkStateUpdate on Element. Did you forget to qref somewhere?
Assignee | ||
Comment 20•12 years ago
|
||
This time with comments
Attachment #546711 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 21•12 years ago
|
||
It's too bad that you can't call the method Element(). At least document that it can't return null?
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #546814 -
Attachment is obsolete: true
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #547517 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 24•12 years ago
|
||
![]() |
Reporter | |
Updated•12 years ago
|
Attachment #552155 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 25•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61094ae6da18
Flags: in-testsuite-
Target Milestone: --- → mozilla11
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61094ae6da18
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•