[FIXr]CreateSheet/LoadSheet issue with mIsSyncLoad

RESOLVED FIXED in mozilla1.3beta

Status

()

P1
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.3beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [whitebox])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

16 years ago
Just thought about this some more and we have a slight problem if someone tries
to use a single loader to load the same sheet as a UA sheet synchronously and as
any sort of sheet asynchronously.  CreateSheet will find the existing load and
clone the sheet, then LoadSheet will synchronously parse into it.

The solution, methinks, is to tell CreateSheet whether the load is sync...
(Assignee)

Updated

16 years ago
Depends on: 107567
Whiteboard: [whitebox]
(Assignee)

Updated

16 years ago
Blocks: 183859

Updated

16 years ago
No longer blocks: 183859

Updated

16 years ago
Blocks: 183859
(Assignee)

Comment 1

16 years ago
Created attachment 108455 [details] [diff] [review]
This should do the job....

This patch has tiny bits of the bug 182124 patch in it...
(Assignee)

Updated

16 years ago
Attachment #108455 - Flags: superreview?(peterv)
Attachment #108455 - Flags: review?(bugmail)
(Assignee)

Updated

16 years ago
Priority: -- → P1
Summary: CreateSheet/LoadSheet issue with mIsSyncLoad → [FIX]CreateSheet/LoadSheet issue with mIsSyncLoad
Target Milestone: --- → mozilla1.3beta
Comment on attachment 108455 [details] [diff] [review]
This should do the job....

r=sicking

However i'm not compleatly happy with the way the |if(!sheet)| tests are being
nested. I dislike code that is too deeply nested since it's hard to read, but
if you think it's worth the extra saved cycles then i won't hold the review for
that.

(don't forget to take the 182124 parts out of there before landing)
Attachment #108455 - Flags: review?(bugmail) → review+
I wonder if another way to fix this would be to merge the two checks for
"existing stylesheets" that we have. I.e. that both ::CreateSheet and
::LoadSheet checks if we have an existing load?
Attachment #108455 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 4

16 years ago
Yeah, merging it would work too.  I still have not found a satisfactory way to
do that (the fact that inline sheets do not go through LoadSheet does not help;
but they also don't get cached so maybe I could just make a copy of the "create
a brand new sheet" code in LoadInlineSheet....).
Summary: [FIX]CreateSheet/LoadSheet issue with mIsSyncLoad → [FIXr]CreateSheet/LoadSheet issue with mIsSyncLoad
(Assignee)

Updated

16 years ago
Attachment #108455 - Flags: approval1.3a?

Comment 5

16 years ago
Comment on attachment 108455 [details] [diff] [review]
This should do the job....

We're trying to wrap up 1.3a and this doesn't look like a blocker. Please hold
until we open up for beta (soon). Thanks
Attachment #108455 - Flags: approval1.3a? → approval1.3a-
(Assignee)

Comment 6

16 years ago
Created attachment 108953 [details] [diff] [review]
de-tangled patch
Attachment #108455 - Attachment is obsolete: true
(Assignee)

Comment 7

16 years ago
fixed.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.