Closed
Bug 34849
Opened 25 years ago
Closed 23 years ago
dynamically added STYLE element doesn't alter style
Categories
(Core :: DOM: CSS Object Model, defect, P3)
Core
DOM: CSS Object Model
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: myk, Assigned: peterv)
References
(Blocks 1 open bug)
Details
(Keywords: dom1, Whiteboard: EDITORBASE-, [DIGBug] [ADT3])
Attachments
(3 files, 7 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
peterv
:
review+
peterv
:
superreview+
roc
:
approval+
|
Details | Diff | Splinter Review |
Overview Description:
Adding a STYLE element to the HEAD of a document via JavaScript does not alter
the style of the document. Here's some code that adds a stylesheet that makes
the background color black and the text color white:
foo = document.createElement('style');
foo.setAttribute('type', 'text/css');
bar = document.createTextNode('BODY { background-color: black; color: white; }');
foo.appendChild(bar);
document.getElementsByTagName('head')[0].appendChild(foo);
Actual Results:
document colors remain the same
Expected Results:
document colors change
Build Date & Platform Bug Found:
Linux 2000-03-27-11
Additional Builds and Platforms Tested On:
Linux 2000-04-05-08
Additional Information:
possibly related to bug 7515
|
Reporter | |
Comment 1•25 years ago
|
||
|
||
Comment 2•25 years ago
|
||
This would be nice to have for beta2, trying M17 for now...
|
|
||
Comment 3•25 years ago
|
||
No way we need this for beta2, removing the keyword (which I added myself).
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not critical to beta2.
Whiteboard: [nsbeta2-]
|
||
Comment 5•24 years ago
|
||
Note related bug 7515 "dynamically adding LINK-ed style sheets does nothing."
|
||
Comment 6•24 years ago
|
||
Since there is no nsbeta2 keyword, the nsbeta2- entry in the status whiteboard
also does not make any sense.
Whiteboard: [nsbeta2-]
|
|
||
Comment 7•24 years ago
|
||
This bug has been marked "future" because the original netscape engineer
workingon this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work in
some way -- please attach your concern to the bug for reconsideration.
OS: Linux → All
Hardware: PC → All
Target Milestone: M17 → Future
|
||
Updated•24 years ago
|
Component: DOM Level 1 → DOM Style
|
|
||
Comment 9•24 years ago
|
||
Taking QA Contact on all open or unverified DOM Style bugs...
QA Contact: janc → ian
|
||
Comment 10•24 years ago
|
||
Ok so there is an ugly workaround (and not very flexible) for this (but you
probably figured it out by now):
************************
<head>
<script type="text/javascript"> document.write("<style> BODY { background-color:
black; } <\/style>");
</script>
</head>
************************
OTOH : what is the problem here? Is it that we don't attempt to repaint after
the style tag has been added to the head tag? Is it that adding a child to the
head tag fails?
Is it possible this is related to bug 18843 "dynamically added SCRIPT not
executable"?
|
||
Comment 11•24 years ago
|
||
The bug is that adding a <style> element does not create an entry in
document.styleSheets. So any styles that the element includes are naturally not
applied...
Blocks: 53030
|
||
Comment 12•23 years ago
|
||
*** Bug 60841 has been marked as a duplicate of this bug. ***
|
|
||
Comment 13•23 years ago
|
||
Solving this bug is important for the CSSization of Composer. When the user
changes embedded css rules in the source view, the changes are not reflected
by Composer when back to normal mode because of the current bug.
Also voting for this bug.
|
|
||
Comment 14•23 years ago
|
||
|
|
||
Comment 15•23 years ago
|
||
Patch v 1.0. Tested successfully with (a) test case in the current bug
(b) Composer adding style rules in the source view and switching back to normal
view to check if the new rules are correctly applied (c) web sites using CSS
embedded and external stylesheets :-)
jst, can you r= please ?
|
|
||
Comment 16•23 years ago
|
||
Just one extra comment : the code is 100% 'inherited' from peterv's fix for
bug 7515, which had r=heikki,harishd, sr=jst...
woohoo! this is very cool!! However, I have two requests:
1. It does not like adding multiple textnodes to the <style>, only the first one
is honored.
2. Changes made to the <style> after it has been added to the tree is ignored
I'll attach a testcase that tests both cases
|
|
||
Comment 19•23 years ago
|
||
I now understand the purpose of the sheetMap in CSSLoaderImpl, this code being
untouched since Peter Linss made it a loooong time ago. It flattens in a single
nsVoidArray all the stylesheets attached to the document wherever they come from;
these sheets can be directly referenced by the doc, or imported by other sheets,
etc. Having a flat array allows the CSSLoader to determine faster where to insert
a sheet.
This code has clearly two problems :
a) the index used in CSSLoaderImpl::InsertSheetInDoc uses a bad index for
insertion of the sheet into the document
b) the sheetMap is never updated when a stylesheet is removed from the document
Working on it and taking ownership of this bug... jst, if you disagree, please
reassign to yourself.
Assignee: jst → glazman
Status: ASSIGNED → NEW
|
|
||
Comment 20•23 years ago
|
||
What I found looking at CSSLoader is absolutely scary : it is a real miracle, or
let's say a coincidence if this code works today and never generated crashes or
leaks. Still working on it.
|
|
||
Comment 21•23 years ago
|
||
Comment on attachment 54854 [details] [diff] [review]
patch v1.0
patch v1.0 obsolete
Attachment #54854 -
Attachment is obsolete: true
|
|
||
Comment 22•23 years ago
|
||
|
|
||
Comment 23•23 years ago
|
||
Comment on attachment 55924 [details] [diff] [review]
patch v 2.0
obsoleting this patch just for making sicking smile ;-)
Attachment #55924 -
Attachment is obsolete: true
|
|
||
Comment 24•23 years ago
|
||
Blocks: 53030
|
|
||
Comment 25•23 years ago
|
||
*** Bug 83787 has been marked as a duplicate of this bug. ***
|
|
||
Comment 26•23 years ago
|
||
new patch for this bug, fix needed by Composer and Transformiix.
patch tested with XPCOM_MEM_LEAK_LOG=1 ; saw nothing special
Jst, sicking, peterv : care to test and review ?
Attachment #55925 -
Attachment is obsolete: true
|
||
Comment 27•23 years ago
|
||
Comment on attachment 66875 [details] [diff] [review]
patch v2.2
To avoid copying the text of the entire stylesheet *twice*, could you change
this code to, instead of using |nsAutoString content|, allocate the nsString
object up front and append directly to it? Also, the return value check
should be |NS_FAILED(rv)| rather than |NS_OK != rv|.
>+ nsCOMPtr<nsIUnicharInputStream> uin;
>+ nsAutoString content;
>+ nsCOMPtr<nsIDOMNode> tcNode;
>+ GetFirstChild(getter_AddRefs(tcNode));
>+ if (!tcNode) return NS_OK;
>+ while (tcNode) {
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(tcNode);
>+ if (!tc) return NS_OK;
Should this be |continue| instead? Or an assertion? Does this
handle CDATA sections?
>+ nsAutoString tcString;
>+ tc->GetData(tcString);
>+ nsCOMPtr<nsIDOMNode> tmp;
>+ tcNode->GetNextSibling(getter_AddRefs(tmp));
>+ tcNode = tmp;
>+ if (tcNode) {
>+ content.Append(PRUnichar('\n'));
>+ }
>+ content.Append(tcString);
>+ }
>+ if (content.IsEmpty()) return NS_OK;
>+ PRBool doneLoading;
>+ rv = NS_NewStringUnicharInputStream(getter_AddRefs(uin), new nsString(content));
>+ if (NS_OK != rv) {
>+ return rv;
>+ }
|
|
||
Comment 28•23 years ago
|
||
Very good catches, thank you David.
About CDATA : this code clearly misses to handle comments !
Producing a new patch asap.
Status: NEW → ASSIGNED
|
|
||
Comment 29•23 years ago
|
||
Attachment #66875 -
Attachment is obsolete: true
I don't think this should be in Future milestone ;) Tentatively moving to 0.9.9.
This is blocking some serious features...
Keywords: nsbeta1
Target Milestone: Future → mozilla0.9.9
|
|
||
Comment 31•23 years ago
|
||
one again : jst, sicking, peterv : reviews/test pls ?
|
||
Comment 32•23 years ago
|
||
I have a few regarding nsHTMLStyleElement.cpp:
in the nsIContent methods, you ignore the rv of the
nsGenericHTMLContainerElement calls, then call UpdateStyleSheet without
catching that rv, and return the rv of the nsGHCE call. Is that wrong, or does
it serve a purpose? Should you update the stylesheets if the nsGHCE call failed?
If you can't get the loader in UpdateStyleSheet, you return NS_OK. I see that
that's copy'n'paste, but is it right?
UpdateStyleSheet shouldn't be inline, IMHO. And at least the indention is
wrong. And cut the line lengths down to something below 80 chars.
You've moved most of the logic from the content-sink to the element, but some
things now live in both places. Checking the src/title/media argument is now
done in both places and checking type is done in content-sink and half-done in
the element (the element doesn't contain the
mimeType.EqualsIgnoreCase("text/css") test that the contentsink does).
IMHO all the logic should live in the element, except that the content-sink
might need to handle the default-style-title. The spec doesn't give any special
meaning to the title, but i'm not sure if there are any special reasons why we
might wanna do so anyway.
I'd prefer if you did
if (!src.IsEmpty())
return NS_OK;
rather then
if (src.IsEmpty()) {
// Whole lot'a code here
}
but that's style.
We should ignore comment-nodes for xhtml <style>s, but that's a different bug.
Iterating using GetNextSibling is O(n^2) and usually get jst a bit upset, but
since this should not be that many nodes we're iterating it might be ok.
+ nsIDocument *oldDoc = mDocument;
+ nsresult rv = nsGenericHTMLContainerElement::AppendChildTo(aKid, aNotify,
+ UpdateStyleSheet(aNotify, oldDoc);
Why not just?
+ nsresult rv = nsGenericHTMLContainerElement::AppendChildTo(aKid, aNotify,
+ UpdateStyleSheet(aNotify, mDocument);
+ nsCOMPtr<nsIDocument> doc;
+ GetDocument(*getter_AddRefs(doc));
just use mDocument
You don't need to handle the case when the parent is a document, <style> should
never be the root element.
You should use aDocIndex when >= 0
since you return rv, would you mind doing |nsresult rv = NS_OK;|? Just to be
safe. (or return NS_OK)
I'm not sure if you should only call nsStyleLinkElement::UpdateStyleSheet only
when src is set. It seems so, but i'm not entierly sure
oh, wait, comments in <style> in html gets created as textnodes, so just QI to
nsIDOMText instead of nsIDOMCharacterData and you should be safe
(nsIDOMCDATASection inherits nsIDOMText so CDATAs will be ok)
You need to remove some things from the xml-contentsink as well: mStyleText and
much of ::ProcessSTYLETag
|
|
||
Comment 36•23 years ago
|
||
nominating this bug for EDITORBASE given its impact on Composer (see Comment #13
below). Peterv's gonna work on it this week-end if he can and I'll take it back
on monday if needed.
Whiteboard: EDITORBASE
|
|
||
Comment 37•23 years ago
|
||
*** Bug 125710 has been marked as a duplicate of this bug. ***
|
|
||
Comment 38•23 years ago
|
||
dup is a DIGBug.
A few thoughts I had while looking over the patch:
1) We're duplicating all the insertion point logic... Wouldn't it make more
sense to make GetStyleSheetInfo set a boolean that indicates inline style and
then make some slight tweaks to nsStyleLinkElement::UpdateStyleSheet to
handle the creation of the string and the calling of ParseInlineStyle?
2) + if (aNotify && mStyleSheet && !doc && aOldDocument) {
This is already handled by nsStyleLinkElement::UpdateStyleSheet
3) + nsString * content = new nsString();
+ if (!tcNode) return NS_OK;
+ if (!tc) return NS_OK;
+ if (content->IsEmpty()) return NS_OK;
+ rv = NS_NewStringUnicharInputStream(getter_AddRefs(uin), content);
+ if (NS_FAILED(rv)) {
+ return rv;
+ }
Any of those returns will leak |content|
4) You should use aDocIndex for the insertion point unless that's -1. The
content sink knows best during page loads....
5) You should not be blocking the parser for alternate inline sheet loads....
currently you are.
Whiteboard: EDITORBASE → EDITORBASE, DIGBug
|
|
||
Comment 39•23 years ago
|
||
Oh, one more problem. If one creates a <style> element and appends it into the
document we should create a stylesheet (an empty)... we do not with that patch
because there are no child text nodes....
|
Assignee | |
Comment 40•23 years ago
|
||
Taking from Daniel. I'll attach a new patch rsn.
Assignee: glazman → peterv
Status: ASSIGNED → NEW
|
|
Assignee | |
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0
oh, according to bug 82829 we ignore comments in <script>s so it should be safe
to do it in <style> too.
|
||
Comment 43•23 years ago
|
||
ADT3 per ADT triage.
Whiteboard: EDITORBASE-, DIGBug → EDITORBASE-, DIGBug [ADT3]
|
|
Assignee | |
Comment 44•23 years ago
|
||
Will probably have a new patch tomorrow. Tracking down a weird bug with a
missing loadgroup for a stylesheet. I doubt I cause it.
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 45•23 years ago
|
||
This one should take care of the comments made. Still going over it myself but
I think it's ready for review.
Attachment #66888 -
Attachment is obsolete: true
// The way we determine the stylesheet's position in the cascade is by lookin
- // at the first of the previous siblings that are style linking elements, and
+ // at the first of the next siblings that are style linking elements, and
// insert just after that one. I'm not sure this is correct for every case fo
change "after" to "before"
+ if (isInline) {
+ PRInt32 count;
+ thisContent->ChildCount(count);
+ if (count < 0)
+ return NS_OK;
return for count <= 0 while you're at it
gotta get sleep, more to come
|
|
Assignee | |
Comment 47•23 years ago
|
||
Regarding the count, see comment 39 and
http://bugzilla.mozilla.org/show_bug.cgi?id=125710. I'll leave count < 0.
It sure would be great to have this for 1.0... I'm itching to a= it!
Comment on attachment 75578 [details] [diff] [review]
patch v2.4
Have you run page load tests?
>Index: base/src/nsStyleLinkElement.cpp
>===================================================================
>- // at the first of the previous siblings that are style linking elements, and
>+ // at the first of the next siblings that are style linking elements, and
You changed this; I don't know enough about the CSS cascading rules to say this
or that but are you sure this is correct?
>+ while (++index && index < count) {
This looks a bit strange. The loop skips case where
index==0, is that intentional? If it is intentional,
please comment why.
In any case, below (either prefix or postfix depending
on which is correct) is how I would have written it and
it is also slightly more efficient as it avoids one test:
+ if (index >= 0) {
+ while (++index < count) {
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(node);
>+ // Ignore nodes that are not DOMText.
>+ if (!tc) {
>+ nsCOMPtr<nsIDOMComment> comment = do_QueryInterface(node);
>+ if (comment)
>+ continue;
>+ else
>+ break;
>+ }
You missed CDATA sections.
>Index: html/content/src/nsHTMLLinkElement.cpp
>===================================================================
>+nsHTMLLinkElement::GetStyleSheetURL(PRBool* aIsInline,
>+ nsAString& aUrl)
> {
>- nsresult rv = NS_OK;
>-
>+ *aIsInline = PR_FALSE;
> aUrl.Truncate();
>+ GetHref(aUrl);
>+ return;
>+}
Please check GetHref() impls; I am pretty sure they also
do Truncate(). If they don't, I think you should fix them
instead.
Also, if GetHref() returns empty for inline stylesheets,
aIsInline parameter is redundant. What does GetHref() do?
I would also suggest adding NS_ENSURE_ARG if aIsInline is
indeed needed in case someone passes a null to us. That
holds true for other functions, like GetStyleSheetInfo()
as well, so please check all new functions.
>Index: html/document/src/nsHTMLContentSink.cpp
>===================================================================
>+ if (NS_OK == rv) {
NS_SUCCEEDED(rv)
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv);
>+ if (NS_OK == rv) {
>+ tc->SetData(content);
>+ }
>+ element->AppendChildTo(text, PR_FALSE, PR_FALSE);
>+ text->SetDocument(mDocument, PR_FALSE, PR_TRUE);
>+ }
I don't think we can have CDATA sections in HTML so this is
probably ok. But there are some people working on supporting
them, dunno if that includes changes in DOM or if we just
convert them to text in the sink/parser.
Other than those it looks really good! It was cool that you noticed some
code reuse opportunities and spotted and fixed some methods that had reinvented
existing methods.
Attachment #75578 -
Flags: needs-work+
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv);
>+ if (NS_OK == rv) {
Oops, one more nit: never ever compare XPCOM method return value to NS_OK
(unless you were copying some old code that may have depended on that). Do
NS_SUCCEEDED or NS_FAILED instead.
|
||
Comment 51•23 years ago
|
||
there's a break after else after continue :-) i'd suggest removing it before
brendan finds it ;-)
|
|
Assignee | |
Comment 52•23 years ago
|
||
> You missed CDATA sections.
Nope: class NS_NO_VTABLE nsIDOMCDATASection : public nsIDOMText
- GetStyleSheetInfo(url, title, type, media, &isAlternate);
+ GetStyleSheetURL(&isInline, url);
+ if (!url.IsEmpty() || isInline) {
+ GetStyleSheetInfo(title, type, media, &isAlternate);
+ }
you could wait with the GetStyleSheetInfo call to right before the
type.EqualsIgnoreCase("text/css") |if| i think.
if (mStyleSheet && url.IsEmpty()) {
// Inline stylesheets have the document's URL as their URL internally
nsCOMPtr<nsIURI> docURL, styleSheetURL;
mStyleSheet->GetURL(*getter_AddRefs(styleSheetURL));
doc->GetBaseURL(*getter_AddRefs(docURL));
if (docURL && styleSheetURL) {
PRBool inlineStyle;
docURL->Equals(styleSheetURL, &inlineStyle);
if (inlineStyle) {
return NS_OK;
}
}
}
This |if| should be removed, afaict it should make any changes to an inline-
stylesheet break, so i suspect there is something that doesn't work. Anyway,
now that we handle inline stylesheets properly it should be gone with.
- // at the first of the previous siblings that are style linking elements, and
+ // at the first of the next siblings that are style linking elements, and
What's the reason for turning this around?
more to come...
nsresult rv = nsGenericHTMLLeafElement::SetAttr(aNameSpaceID, aName,
aValue, aNotify);
- UpdateStyleSheet(aNotify);
+ if (NS_SUCCEEDED(rv)) {
+ UpdateStyleSheet();
+ }
this'll make UpdateStyleSheet be called with aOldDocument==null, so we won't
remvoe the stylesheet from the document at the top of UpdateStyleSheet. Does
the CSS-loader handle this for us or?
in nsHTMLLinkElement:
nsresult rv = nsGenericHTMLLeafElement::SetAttr(aNameSpaceID, aName,
aValue, aNotify);
- UpdateStyleSheet(aNotify);
+ if (NS_SUCCEEDED(rv)) {
+ UpdateStyleSheet();
+ }
should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same in UnSetAttr
+nsHTMLLinkElement::GetStyleSheetURL(PRBool* aIsInline,
+ nsAString& aUrl)
{
- nsresult rv = NS_OK;
-
+ *aIsInline = PR_FALSE;
aUrl.Truncate();
+ GetHref(aUrl);
+ return;
+}
the old code had some base-url handling:
- nsCOMPtr<nsIURI> baseURL;
- GetBaseURL(*getter_AddRefs(baseURL));
- rv = NS_MakeAbsoluteURI(aUrl, href, baseURL);
which i think you still need, same in nsHTMLStyleElement
+ if (!*aIsAlternate && !aTitle.IsEmpty()) { // possibly preferred sheet
+ nsAutoString prefStyle;
+ mDocument->GetHeaderData(nsHTMLAtoms::headerDefaultStyle, prefStyle);
+
+ if (prefStyle.IsEmpty()) {
+ mDocument->SetHeaderData(nsHTMLAtoms::headerDefaultStyle, aTitle);
IMHO this shouldn't live in a GetFoo-type function, however since it did in the
old code too it's ok to live with it for now. However I'm a bit worried that
since you've split it up into two functions the second might not always be
called, and therefor the code isn't always ran. Please check if that could be a
problem.
in nsHTMLStyleElement
+ NS_IMETHOD InsertChildAt(nsIContent* aKid, PRInt32 aIndex, PRBool aNotify,
PRBool aDeepSetDocument)
+ {
+ nsresult rv = nsGenericHTMLContainerElement::InsertChildAt(aKid, aIndex,
aNotify, aDeepSetDocument);
+ if (NS_SUCCEEDED(rv)) {
+ UpdateStyleSheet();
+ }
should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same applies to the other
child-add/replace functions
+nsHTMLStyleElement::GetStyleSheetURL(PRBool* aIsInline,
+ nsAString& aUrl)
{
trunkate aUrl in the beginning in case it's an inline-stylesheet.
just the contentsink left
+ if (NS_OK == rv) {
+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv);
+ if (NS_OK == rv) {
tssk tssk, use NS_SUCCEEDED instead. And use |if (tc)| on the second.
+ tc->SetData(content);
i think AppendData is faster.
+ element->AppendChildTo(text, PR_FALSE, PR_FALSE);
+ text->SetDocument(mDocument, PR_FALSE, PR_TRUE);
does AppendChildTo call SetDocument for you?
- nsresult ProcessSTYLETag();
WOOHOO!! :-)
can you remove mStyleElement too?
ok, i'm done
|
|
Assignee | |
Comment 57•23 years ago
|
||
> this'll make UpdateStyleSheet be called with aOldDocument==null, so we won't
> remvoe the stylesheet from the document at the top of UpdateStyleSheet. Does
> the CSS-loader handle this for us or?
We don't want to remove it from an old document, that should only happen when
the element is removed froma document or moved to a different document.
> should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same in UnSetAttr
aNotify shouldn't affect stylesheet loading.
I've verified that all the url getters correctly use the base url. No need to do
it twice.
Other comments addressed as discussed. New patch coming up.
|
|
Assignee | |
Comment 58•23 years ago
|
||
Attachment #75578 -
Attachment is obsolete: true
- if (aNotify && mStyleSheet && !doc && aOldDocument) {
+ if (mStyleSheet && !doc && aOldDocument) {
i think you should remove the !doc too. If an element is moved from one
document to another doc will point to the new document, but we still want to
remove it from the old document.
- if (NS_OK == rv) {
+ if (NS_SUCCEEDED(rv)) {
nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv);
- if (NS_OK == rv) {
+ if (tc) {
please remove the &rv argument to the QI
+ nsCOMPtr<nsIContent> content = do_QueryInterface(element);
+ if (content &&
+ !content->HasAttr(kNameSpaceID_None, nsHTMLAtoms::src)) {
// The skipped content contains the inline style data
const nsString& content = aNode.GetSkippedContent();
IMHO it would be nicer if you always got the skipped content and |if|ed on it
being IsEmpty instead. But that's optional
with those r=sicking
Attachment #76347 -
Flags: review+
|
|
||
Comment 60•23 years ago
|
||
Comment on attachment 76347 [details] [diff] [review]
patch v2.5
- If performance of nsParserUtils::GetQuotedAttributeValue() matters it could
be made faster if it returned an nsDependentSubstring by value, thus avoiding
the copying of the attribute value.
I didn't really find anything wrong with the patch worth mentioning here
(except for some unnecessary .get()'s on some nsCOMPtr's), so sr=jst
Attachment #76347 -
Flags: superreview+
|
|
Assignee | |
Comment 61•23 years ago
|
||
I am currently doing performance tests. I'll attach the latest patch tomorrow
and request a= then.
Speaking as one driver, I'm eager to approve this :-).
|
|
Assignee | |
Comment 63•23 years ago
|
||
Pageload numbers without patch:
Test id: 3CAAF14401
Avg. Median : 2638 msec Minimum : 606 msec
Average : 2650 msec Maximum : 9219 msec
Pageload numbers with patch:
Test id: 3CAACD35E1
Avg. Median : 2667 msec Minimum : 597 msec
Average : 2684 msec Maximum : 9147 msec
|
|
Assignee | |
Comment 64•23 years ago
|
||
Attachment #76347 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 65•23 years ago
|
||
Comment on attachment 77441 [details] [diff] [review]
patch v2.6
Carrying forward r and sr.
Attachment #77441 -
Flags: superreview+
Attachment #77441 -
Flags: review+
|
|
Assignee | |
Comment 66•23 years ago
|
||
Ran through Hixie's import test suite (basic and evil), no regressions. Surfed
around a bit and looked for leaks, I am not seeing more leaks after the patch.
|
||
Comment 67•23 years ago
|
||
Peter did you run the layout regression tests?
http://www.mozilla.org/newlayout/regress.html
catch me on IRC if you need help.
|
|
Assignee | |
Comment 68•23 years ago
|
||
No, I did not. This patch only affects loading of stylesheets and Hixie's test
suite does extensive testing of that. The patch touches nothing in layout.
That is about 1% perf hit on page load, but peterv is located in France and run
the tests just once. I am compiling an optimized build and will run the tests
locally several times to see if peterv's result was noise.
In the mean time, try to see if there is anything that could cause the slowdown
(if it is real) and how to improve.
I can't really find any sigificant difference in the amount of work we do. In
fact most changes should make us *faster*. One easy way to save a few cycles
would be to make UpdateStyleSheet non-virtual in nsStyleLinkElement.h, that way
it is call non-virtually from all nodes that inherit nsStyleLinkElement
The last patch is strange, did you edit it by hand? It does not apply cleanly on
Unix/Windows; it appears it is 3 diffs concatenated because there are 3
different "root" directories. It was easy to fix, but still... wanna make sure
you did not miss anything.
Then the perf numbers (RH Linux 7.2, 384MB RAM, 500MHz, opt build):
TRUNK1
Test id: 3CAB96B4C9
Avg. Median : 1265 msec Minimum : 228 msec
Average : 1296 msec Maximum : 4654 msec
TRUNK2
Test id: 3CAB996952
Avg. Median : 1258 msec Minimum : 228 msec
Average : 1269 msec Maximum : 4228 msec
TRUNK3
Test id: 3CAB9F9AB4
Avg. Median : 1262 msec Minimum : 227 msec
Average : 1273 msec Maximum : 4234 msec
STYLE1
Test id: 3CABA8D74E
Avg. Median : 1260 msec Minimum : 216 msec
Average : 1287 msec Maximum : 4526 msec
STYLE2
Test id: 3CABAB0E55
Avg. Median : 1256 msec Minimum : 225 msec
Average : 1269 msec Maximum : 4271 msec
STYLE3
Test id: 3CABAD0336
Avg. Median : 1265 msec Minimum : 233 msec
Average : 1275 msec Maximum : 4244 msec
Trunk avg med 1262
Trunk avg avg 1279
Style avg med 1260 (0.2% improvement)
Style avg avg 1277 (0.2% improvement)
|
|
Assignee | |
Comment 72•23 years ago
|
||
Thanks Heikki. Yeah, the diff was concatenated manually, MacCVS usually doesn't
care about that. I try to make sure that the diffs are correct for other
platforms too ;-) but I forgot to attach that one here.
Keywords: adt1.0.0
Comment on attachment 77441 [details] [diff] [review]
patch v2.6
a=roc+moz
woohoo!
Attachment #77441 -
Flags: approval+
|
||
Comment 74•23 years ago
|
||
adt1.0.0+ (on ADT's behalf) for checkin approval into 1.0, pending greenlight
Gerardo.
Test windows opt build with this and P3P:
http://green.nscp.aoltw.net/heikki/mozilla-style-p3p-20020403.tgz
|
||
Comment 76•23 years ago
|
||
no bugs out of the ordinary for smoketests on the above build (same as this
mornings build that is)
Heikki, is there a particular test page to make certain this works on?
Well, there are several test cases which don't work without this patch. But you
don't need to test those: they work (otherwise we would not be checking this
in). Thanks for testing!
Gerardo said this is ok.
He will also get people to do extensive testing as soon as you have checked in
so we will notice regressions soon if there are any.
|
||
Comment 79•23 years ago
|
||
It's pretty obvious to determine if the fix didn't work, because style wouldn't
be applied on any page, or even the UI. We think the fix can be checked-in now.
We'll be running CSS and Layout tests on 4/6. According to Heikki, if any
regression is found it'll be easy to pull back the code.
|
|
Assignee | |
Comment 80•23 years ago
|
||
Checked in. Pageload on btek went up slightly and went down slightly on
sleestack. I'll take that as a yes.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
|
|
||
Comment 81•23 years ago
|
||
PETER, YOU RULE !!!!!
|
||
Comment 82•23 years ago
|
||
After this bug was checked in I tested the tables and frames with CSS for
regressions. It appears that this fix dint cause any regression in these areas.
You need to log in
before you can comment on or make changes to this bug.
Description
•