Closed Bug 582712 Opened 14 years ago Closed 14 years ago

remove nsHTMLDocument::TryBookmarkCharset

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: Gavin, Assigned: Ms2ger)

References

Details

Attachments

(1 file, 2 obsolete files)

It's one of the last steps we try when looking for charset data, querying the places database for existing bookmarks. I suspect that hitting places at that point in the loading process causes more perf damage than the benefit gained, especially since the "user selected the character set and then bookmarked the page" scenario seems relatively unlikely.
Assignee: nobody → Ms2ger
Blocks: 337970
Attached patch WIP (obsolete) — Splinter Review
Still need to test this.
Attached patch Patch v1 (obsolete) — Splinter Review
Looks like there were no tests for this case.
Attachment #461265 - Attachment is obsolete: true
Attachment #462083 - Flags: review?
Attachment #462083 - Flags: review? → review?(gavin.sharp)
Comment on attachment 462083 [details] [diff] [review]
Patch v1

I'm not a good reviewer for this - need a DOM peer. Would also like bz to at least sign off. Can you get useful Tp numbers from try?
Attachment #462083 - Flags: review?(gavin.sharp)
Attachment #462083 - Flags: review?(bzbarsky)
I don't have access to try.
Comment on attachment 462083 [details] [diff] [review]
Patch v1

Yeah, ok.
Attachment #462083 - Flags: review?(bzbarsky) → review+
Attachment #462083 - Flags: approval2.0?
Do we know what the perf win is from this? It doesn't seem highly risky, but we're locking down approvals much more tightly now.
Blocks: 603470
this is probably causing Places to init earlier than needed and through bookmarks service. And it's causing content process to init Places as well. I'd really appreciate approval on this patch.
Also it is causing sync stuff to happen on page load (UI hang), when we have tried to make all that stuff async for FX4.
Blocks: 606157
Status: NEW → ASSIGNED
Attachment #462083 - Flags: approval2.0? → approval2.0+
Attachment #462083 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8c94cb4ac9d0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: