Last Comment Bug 713825 - Move nsIDocCharset and nsIDocumentCharsetInfo into docshell
: Move nsIDocCharset and nsIDocumentCharsetInfo into docshell
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 721533 721535
Blocks: 711790 720310 720312 720952
  Show dependency treegraph
 
Reported: 2011-12-28 02:09 PST by Simon Montagu :smontagu
Modified: 2012-04-24 21:19 PDT (History)
5 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move nsIDocCharset (15.34 KB, patch)
2011-12-28 07:27 PST, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Review
Move nsIDocumentCharsetInfo (39.43 KB, patch)
2011-12-28 07:28 PST, Simon Montagu :smontagu
bzbarsky: review+
mbrubeck: review+
Details | Diff | Review
Remove forcedDetector (3.78 KB, patch)
2011-12-28 10:48 PST, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Review

Description Simon Montagu :smontagu 2011-12-28 02:09:55 PST

    
Comment 1 Simon Montagu :smontagu 2011-12-28 07:27:57 PST
Created attachment 584569 [details] [diff] [review]
Move nsIDocCharset
Comment 2 Simon Montagu :smontagu 2011-12-28 07:28:58 PST
Created attachment 584570 [details] [diff] [review]
Move nsIDocumentCharsetInfo
Comment 3 Simon Montagu :smontagu 2011-12-28 10:48:56 PST
Created attachment 584597 [details] [diff] [review]
Remove forcedDetector

Since the forcedDetector thing has been "write me" since for ever, I should think it can just disappear.
Comment 4 Boris Zbarsky [:bz] 2011-12-28 12:42:42 PST
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 [:bz] 2011-12-28 12:48:36 PST
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 [:bz] 2011-12-28 12:49:04 PST
Oh, that second part probably also needs documentation, but seems to not be used by addons much.
Comment 7 Boris Zbarsky [:bz] 2011-12-28 12:50:39 PST
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 2011-12-28 22:54:09 PST
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) 2011-12-29 10:29:37 PST
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 2012-01-16 22:01:42 PST
(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 2012-01-16 22:03:08 PST
"an error" -- i.e. an error message that makes the needed fix clear to a web developer.
Comment 12 Boris Zbarsky [:bz] 2012-01-16 22:24:15 PST
> 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 15 Eric Shepherd [:sheppy] 2012-04-24 11:00:04 PDT
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] 2012-04-24 11:04:20 PDT
(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 2012-04-24 21:19:40 PDT
(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 :)

Note You need to log in before you can comment on or make changes to this bug.