Move nsIDocCharset and nsIDocumentCharsetInfo into docshell

RESOLVED FIXED in mozilla12

Status

()

Core
Internationalization
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla12
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 584569 [details] [diff] [review]
Move nsIDocCharset
Attachment #584569 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 years ago
Created attachment 584570 [details] [diff] [review]
Move nsIDocumentCharsetInfo
Attachment #584570 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

6 years ago
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.
Attachment #584597 - Flags: review?(bzbarsky)

Comment 4

6 years ago
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.
Attachment #584569 - Flags: review?(bzbarsky) → review+

Comment 5

6 years ago
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...
Attachment #584570 - Flags: review?(bzbarsky) → review+

Comment 6

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

Comment 7

6 years ago
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
Attachment #584597 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

6 years ago
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.
Attachment #584570 - Flags: review?(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.
Attachment #584570 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

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

Comment 12

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Blocks: 720310
(Assignee)

Updated

6 years ago
Blocks: 720312
(Assignee)

Comment 13

6 years ago
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
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
(Assignee)

Updated

6 years ago
Keywords: addon-compat, dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/1cbdfc84d7e3
https://hg.mozilla.org/mozilla-central/rev/57f0b07c2dc5
https://hg.mozilla.org/mozilla-central/rev/f173a9a1e056
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 720952
Depends on: 721533
Depends on: 721535
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.
Keywords: dev-doc-needed → dev-doc-complete
(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
(Assignee)

Comment 17

5 years ago
(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 :)
You need to log in before you can comment on or make changes to this bug.