582712 - remove nsHTMLDocument::TryBookmarkCharset

Status: RESOLVED FIXED

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

Description

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

Still need to test this.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Looks like there were no tests for this case.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't have access to try.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462083 [details] [diff] [review]
Patch v1

Yeah, ok.

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8c94cb4ac9d0