201821 - fix nsHTMLDocument.cpp so it doesn't depend on nsIBookmarksService.idl

Status: RESOLVED DUPLICATE of bug 101995

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

Description

[Brendan Eich [:brendan]]

See bug 201809 comment 6.  I'd like to make it so nsHTMLDocument.cpp does not
include any header file exported by the XPFE (or any other front end!).  I think
nsHTMLDocument is the sole offender, due to bug 18022's fix:

$ find content -name Makefile.in -print | xargs egrep -l 'appcomps|xpfe'
content/html/document/src/Makefile.in

Options:

1) Move nsIBookmarksService.idl to some place as primitive, or more primitive,
than content.

2) Make a new interface that XPFE and other FEs can implement, that has just the
one needed lastCharset attribute (which seems misnamed to me).

3) Move the lastCharset attribute or a C++-only GetLastCharset method to some
existing interface that Gecko already provides for FEs to implement.

Is there such an interface as 3?  If so, that wins.  Otherwise, I'll do 2.  Pls.
advise.

/be

Comment 1

[Brendan Eich [:brendan]]

Alec, is this something that matters for minimo?

/be

Comment 2

[Brian Ryner (not reading)]

I hacked around this in mozilla/Makefile.in to unbreak the build, by adding
browser/components/bookmarks/public to tier_9_dirs for phoenix.

Comment 3

[Scott MacGregor]

I'm interested in this as well for Thunderbird. I have to work around the fact
that bookmarks is not part of thunderbird, yet nsHTMLDocument.cpp expects
nsIBookmarkservice to be there. Currently I export nsIBookmarkService.idl, but
that leads to assertions when layout trys to create the bookmarks service and
can't. It sure would be great if nsHTMLDocument.cpp didn't have this dependency. 

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Storing the charset in the bookmarks is badly broken already (see bug 174197). 
Perhaps this feature should just be removed?  Having state stored in bookmarks
influence how a page is displayed seems like it's just asking for problems.

Comment 5

[Brendan Eich [:brendan]]

Simon, can we simply remove the nsHTMLDocument::TryBookmarkCharset method and
nsIBookmarksService.h dependency altogether, at least as the first step in a two
part fix where something else that doesn't create a bogus dependency of Gecko on
the front end happens after?

/be

Comment 6

[Johnny Stenback (:jst)]

This seems very very backwards to me. Even if we do want to keep storing charset
data in bookmarks (which I don't think we should), shouldn't the bookmarks code
then tell gecko what charset to prefer when loading a bookmark, rather than
having gecko ask the bookmarks service when it's loading a URL?

I say rip this out, unless someone can prove this useful.

Comment 7

[Alec Flett]

sorry to jump in so late - this is pretty much a dupe of bug 101995, which has
been unfortunately assigned to me for ages. The idea is that we want to try
every possible location to guess at the charset - on the i18n side of things we
are known for being really smart about getting the charset right even on broken
sites.  I don't see anything wrong with storing the charset in bookmarks, we
just need to do it in a generic way. 

Case in point: suppose you manually determine the charset for a site, and need a
persistent place to store that decision. Where better than in bookmarks?

Please look at the surrounding code. TryBookmarkCharset is just one of many
places that we look for a charset override, including the cache, charset hints,
parent document, and so forth. see 
http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLDocument.cpp#910

All of these methods should use the generic method that I've outlined in bug 101995.

Comment 8

[Brendan Eich [:brendan]]

Bryner has had to hack all over creation to work around the botch that Gecko
depends on the FE.  Alec, I want this bug fixed now, this week.  If you can't
fix bug 101995 this week, then I'd like to do something expedient.  The question
is what?  It's ridiculous that bryner, mscott, and dozens of others (the Galeon
and Camino hackers, e.g.) have had to spend time dealing with this.

/be

Comment 9

[Alec Flett]

I agree, its sucks and I've wanted to fix it for a while.. but as usual I've got
a lot on my plate and I'm not sure I'll be able to fix bug 101995 this week. 

It seems like if expediency is the issue, this should be an #ifdef..I think
putting any more effort than and #ifdef is a waste of time, unless it is towards
fixing bug 101995.

Comment 10

[Brian Ryner (not reading)]

brendan, any progress on this?

Comment 11

[Brendan Eich [:brendan]]

Alecf is all over the problem reported here, in bug 101995.  There doesn't seem
to be a shorter-term fix than his work there, which should land now that
1.5alpha is open for changes.  I'm marking this bug a dup.

/be

*** This bug has been marked as a duplicate of 101995 ***