713825 - Move nsIDocCharset and nsIDocumentCharsetInfo into docshell

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Comment 1

[Simon Montagu :smontagu]

Attachment: Move nsIDocCharset

Comment 2

[Simon Montagu :smontagu]

Attachment: Move nsIDocumentCharsetInfo

Comment 3

[Simon Montagu :smontagu]

Attachment: Remove forcedDetector

Since the forcedDetector thing has been "write me" since for ever, I should think it can just disappear.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 584569 [details] [diff] [review]
Move nsIDocCharset

There are a bunch of addons that use nsIDocCharset.  So this needs to be dev-doc-needed and addon-compat, and we may want to leave an empty nsIDocSharset interface that docshell QIs to and window returns via getInterface for a release or two.....

r=me modulo that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 584570 [details] [diff] [review]
Move nsIDocumentCharsetInfo

Can you document the new properties on nsIDocShell, please?

Are you sure the mobile browser.xml change won't screw things up?  Seems like people could be relying on that .docShell prop returning null.  Needs review from someone who knows that code.

r=me with that...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, that second part probably also needs documentation, but seems to not be used by addons much.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 584597 [details] [diff] [review]
Remove forcedDetector

Seems like BrowserSetForcedDetector should be renamed and any calls with !doReload be eliminated.  Followup bug?  r=me

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 584570 [details] [diff] [review]
Move nsIDocumentCharsetInfo

(In reply to Boris Zbarsky (:bz) from comment #5)
> Are you sure the mobile browser.xml change won't screw things up?  Seems
> like people could be relying on that .docShell prop returning null.  Needs
> review from someone who knows that code.

Matt, can you look at the changes to mobile/ ? Thanks.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 584570 [details] [diff] [review]
Move nsIDocumentCharsetInfo

>+++ b/mobile/xul/chrome/content/bindings/browser.xml

>       <property name="docShell"
>                 onget="return null"
>-                readonly="true"/>
>+                readonly="true">
>+         <getter><![CDATA[
>+            return {
>+               forcedCharset : this._charset,
>+               forcedDetector : false,
>+               parentCharset : "",
>+               parentCharsetSource : 0
>+            }
>+         ]]></getter>
>+      </property>

Please also remove the onget="return null" line.

Aside from that, this looks good to me.  No mobile code accesses the browser.docShell property.

Comment 10

[Simon Montagu :smontagu]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 584569 [details] [diff] [review]
> Move nsIDocCharset
> 
> There are a bunch of addons that use nsIDocCharset.  So this needs to be
> dev-doc-needed and addon-compat, and we may want to leave an empty
> nsIDocSharset interface that docshell QIs to and window returns via
> getInterface for a release or two.....

I don't understand this, neither (a) technically nor (b) the principals behind it. For (a), it would probably help if you could point me to some other interface where we did something similar. For (b) I don't understand why an empty interface is better for compat than no interface -- I would have thought the reverse (if those are the only choices), since I would expect no interface to give an error but an empty interface to produce hard-to-debug bugs.

Comment 11

[Simon Montagu :smontagu]

"an error" -- i.e. an error message that makes the needed fix clear to a web developer.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> For (a), it would probably help if you could point me to some other interface where we
> did something similar

I can't find it right now, sadly....  There was a case recently where we brought back an interface as empty precisely for extension compat.

> (b) I don't understand why an empty interface is better for compat than no interface

Because of XPConnect interface flattening.

Say you have code that does something like:

  var foo = getBar().QueryInterface(Ci.nsIBaz).something();

If |something| is moved from nsIBaz to nsIBar (which is the return value of getBar()) and nsIBaz is eliminated, this line of code will throw because the QI will fail.  But if the same move is done but nsIBaz is left empty, the code will work, because in JS the return value of the QI has all the nsIBar _and_ nsIBaz methods visible on it.

In this case, as long as the extension was getting nsIDocumentCharsetInfo by QIing an nsIDocShell object, the extension will keep working.

Comment 13

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbdfc84d7e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f0b07c2dc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aab7ae07561

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1cbdfc84d7e3
https://hg.mozilla.org/mozilla-central/rev/57f0b07c2dc5
https://hg.mozilla.org/mozilla-central/rev/f173a9a1e056

Comment 15

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/XUL/Property/documentCharsetInfo
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDocShell

Mentioned on Firefox 12 for developers.

Comment 16

[Eric Shepherd [:sheppy]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> > For (a), it would probably help if you could point me to some other interface where we
> > did something similar
> 
> I can't find it right now, sadly....  There was a case recently where we
> brought back an interface as empty precisely for extension compat.
> 
> > (b) I don't understand why an empty interface is better for compat than no interface
> 
> Because of XPConnect interface flattening.

We did this with nsIDOMWindowInternal in Firefox 8. See: https://developer.mozilla.org/en/Firefox/Updating_add-ons_for_Firefox_8#Interfaces_have_been_merged

Comment 17

[Simon Montagu :smontagu]

(In reply to Eric Shepherd [:sheppy] from comment #16)
> We did this with nsIDOMWindowInternal in Firefox 8. See:
> https://developer.mozilla.org/en/Firefox/Updating_add-
> ons_for_Firefox_8#Interfaces_have_been_merged

Thanks, I managed to work this out and make the necessary changes before checkin :)