385559 - Remembering zoom for specific data: URLs is kinda silly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/20070622 Minefield/3.0a6pre

1. Visit data:text/html,foo
2. Cmd++ a few times to increase the text size.
3. Visit data:text/html,bar
      Result: "bar" is displayed at normal size.
4. Visit data:text/html,foo
      Result: "foo" is displayed at a large size.

IMO, Firefox should 
A) not remember text zoom for data: URLs at all.
B) treat data: URLs as part of a single "data:" site.
C) treat data: URLs as part of the site they "came from", like security code does.

Remembering it for a specific data: URL is really weird.

Comment 1

[Jesse Ruderman]

Argh!  Now when I try to (un)zoom some data: URLs, I get:

JavaScript error: , line 0: uncaught exception: 2147942487

Comment 2

[Martijn Wargers (dead)]

Personally, I actually think this is useful.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Jesse and Martijn: can you give a couple real-world use cases where you run into these data: URLs and why the current behavior doesn't (or does, for Martijn) do what you want?  I don't have a good sense of when such data: URLs are typically encountered and what behavior makes sense for them.

Comment 4

[Jesse Ruderman]

I use them for one-off tests.  For example, I was playing with ligatures recently, so I typed in URLs like:

data:text/html,fi
data:text/html,<span>f</span><span>i</span>
data:text/html,<div style="font: 80px zapfino">Zapfino

Comment 5

[Martijn Wargers (dead)]

I mainly use a data url with the data uri kitchen:
http://simonwillison.net/2003/Aug/11/selfContained/
Afaict, data url's are only something for web geeks, not for normal people, so I don't think this is a big issue.
If you want to implement what Jesse says that's fine by me.

Comment 6

[Mike Connor [:mconnor]]

For data: I think I'm going with a), but I don't think this blocks, its edgecasey at best.

Comment 7

[Mike Connor [:mconnor]]

Not a blocker.  Myk, if you want to exclude data: that's up to you, I think its probably for the best, but I'm pretty ambivalent since so few people use them standalone.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

would it make sense to patch ContentPrefService instead?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

> would it make sense to patch ContentPrefService instead?

Yeah, I think it would, given that the service is doing hostname-based checks, and there is no hostname for data: URLs.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 286922 [details] [diff] [review]
patch v2

>Index: browser/base/content/browser-textZoom.js

>     // Register ourselves with the sink so we know when the location changes.
>     var globalValue = ContentPrefSink.addObserver(this.name, this);
>-    this.globalValue = this._ensureValid(globalValue);
>+    if (globalValue)
>+      this.globalValue = this._ensureValid(globalValue);

It's not clear how this change is related to this bug, but this should get added to _ensureValid instead, and it should check for the undefined value instead of testing for boolean truth, since that's what the getPref call in ContentPrefSink.addObserver returns if there is no global value, i.e.:

  _ensureValid: function (aValue) {
    if (typeof aValue == "undefined" || isNaN(aValue))
      return 1;


>@@ -252,18 +253,16 @@ var FullZoom = {
>   _applyPrefToSetting: function (aValue) {
>     // Bug 375918 means this will sometimes throw, so we catch it
>     // and don't do anything in those cases.
>     try {
>       if (typeof aValue != "undefined")
>         ZoomManager.fullZoom = this._ensureValid(aValue);
>       else if (typeof this.globalValue != "undefined")
>         ZoomManager.fullZoom = this.globalValue;
>-      else
>-        ZoomManager.reset();

Maybe I'm missing something, but I don't understand the reason for this change.  If we don't reset the zoom, and the previous page had a zoom applied, that zoom will apply to the new page, although it shouldn't, no?


>Index: toolkit/components/contentprefs/src/nsContentPrefService.js

>     var currentValue = this.getPref(aURI, aName);
>     if (typeof currentValue != "undefined" && currentValue == aValue)
>       return;
> 
>     var settingID = this._selectSettingID(aName) || this._insertSetting(aName);
>     var group, groupID, prefID;
>     if (aURI) {
>       group = this.grouper.group(aURI);
>+      if (!group)
>+        return;

I wonder if this should throw to indicate that it couldn't set the preference.  That would complicate the calling code but make the method's behavior less opaque.


>   _selectPref: function ContentPrefService__selectPref(aGroup, aSetting) {
>+    if (!aGroup)
>+      return undefined;

Same question here.


>+    switch (aURI.scheme) {
>+      case "data":
>+      case "javascript":
>+        // Empty group, prefs won't be stored.
>+        return "";
>+    }
>+

Seems like this method should return null or undefined rather than the empty string if the URI couldn't be grouped by hostname because there isn't a hostname in the URI (either that or it should throw NS_ERROR_NOT_AVAILABLE).

Also, I wonder if it makes more sense to whitelist schemes that we know provide hostnames (i.e. http, https) instead of blacklisting schemes that we know don't provide them.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> (From update of attachment 286922 [details] [diff] [review])
> >Index: browser/base/content/browser-textZoom.js
> 
> >     // Register ourselves with the sink so we know when the location changes.
> >     var globalValue = ContentPrefSink.addObserver(this.name, this);
> >-    this.globalValue = this._ensureValid(globalValue);
> >+    if (globalValue)
> >+      this.globalValue = this._ensureValid(globalValue);
> 
> It's not clear how this change is related to this bug,

If addObserver doesn't return a global value, why should we set it to 1? That would mean that if you load a data URI into tab A, zoom in, switch to tab B and back to A, the previous zoom level would be lost.

Of course, if addObserver does return a global value, we would still lose the zoom level. I guess I have a general problem with globalValue.

> but this should get
> added to _ensureValid instead, and it should check for the undefined value
> instead of testing for boolean truth, since that's what the getPref call in
> ContentPrefSink.addObserver returns if there is no global value, i.e.:
> 
>   _ensureValid: function (aValue) {
>     if (typeof aValue == "undefined" || isNaN(aValue))
>       return 1;

isNaN is already there, and isNaN(undefined) is true.

> >       if (typeof aValue != "undefined")
> >         ZoomManager.fullZoom = this._ensureValid(aValue);
> >       else if (typeof this.globalValue != "undefined")
> >         ZoomManager.fullZoom = this.globalValue;
> >-      else
> >-        ZoomManager.reset();
> 
> Maybe I'm missing something, but I don't understand the reason for this change.

See the use case from above -- if there's no stored value, we don't just want to reset the zoom.

>  If we don't reset the zoom, and the previous page had a zoom applied, that
> zoom will apply to the new page, although it shouldn't, no?

Yes, that's probably true.

> >Index: toolkit/components/contentprefs/src/nsContentPrefService.js
> 
> >     var currentValue = this.getPref(aURI, aName);
> >     if (typeof currentValue != "undefined" && currentValue == aValue)
> >       return;
> > 
> >     var settingID = this._selectSettingID(aName) || this._insertSetting(aName);
> >     var group, groupID, prefID;
> >     if (aURI) {
> >       group = this.grouper.group(aURI);
> >+      if (!group)
> >+        return;
> 
> I wonder if this should throw to indicate that it couldn't set the preference. 
> That would complicate the calling code but make the method's behavior less
> opaque.

Could make sense, although I'd prefer to not change the interface.

> >   _selectPref: function ContentPrefService__selectPref(aGroup, aSetting) {
> >+    if (!aGroup)
> >+      return undefined;
> 
> Same question here.

_selectPref is only used internally by getPref, where undefined is actually part of the idl comment: "Besides the regular string, integer, boolean, etc. values, this method may return [...] undefined [...], which means there is no record for this pref in the database."

> >+    switch (aURI.scheme) {
> >+      case "data":
> >+      case "javascript":
> >+        // Empty group, prefs won't be stored.
> >+        return "";
> >+    }
> >+
> 
> Seems like this method should return null or undefined rather than the empty
> string if the URI couldn't be grouped by hostname because there isn't a
> hostname in the URI (either that or it should throw NS_ERROR_NOT_AVAILABLE).

"" instead of null is a bit of a hack, as nsIContentURIGrouper::group claims to return a string.

> Also, I wonder if it makes more sense to whitelist schemes that we know provide
> hostnames (i.e. http, https) instead of blacklisting schemes that we know don't
> provide them.

That might be too restrictive. I don't know all the use cases for content preferences.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 287275 [details] [diff] [review]
patch v3

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 286922 [details] [diff] [review] [details])
> > >Index: browser/base/content/browser-textZoom.js
> > 
> > >     // Register ourselves with the sink so we know when the location changes.
> > >     var globalValue = ContentPrefSink.addObserver(this.name, this);
> > >-    this.globalValue = this._ensureValid(globalValue);
> > >+    if (globalValue)
> > >+      this.globalValue = this._ensureValid(globalValue);
> > 
> > It's not clear how this change is related to this bug,
> 
> If addObserver doesn't return a global value, why should we set it to 1? That
> would mean that if you load a data URI into tab A, zoom in, switch to tab B and
> back to A, the previous zoom level would be lost.

Good point, this makes sense now: we only define the global value property if we've got a global value stored in the database.  Otherwise we leave it undefined.

Nit: the conditional should be |typeof globalValue != "undefined"|, though.  It doesn't really matter here, as the global value should never resolve to a false value if defined, but it could matter in other prefs, and if folks making other site-specific prefs copy this code, they could introduce a bug.


> Of course, if addObserver does return a global value, we would still lose the
> zoom level. I guess I have a general problem with globalValue.

The problem is that we're hanging the setting of the zoom off the location change event, which means we potentially set the zoom multiple times per tab.  Using the location change event has other problems, like bug 386835.

We need to hang the setting of the zoom off an event that fires once per top-level document load.  As far as I can tell, that's the only way to really fix this problem.  Unfortunately, I'm not sure what that event would be.


> > it should check for the undefined value
> > instead of testing for boolean truth, since that's what the getPref call in
> > ContentPrefSink.addObserver returns if there is no global value, i.e.:
> > 
> >   _ensureValid: function (aValue) {
> >     if (typeof aValue == "undefined" || isNaN(aValue))
> >       return 1;
> 
> isNaN is already there, and isNaN(undefined) is true.

Ah, right, ok.


> > >Index: toolkit/components/contentprefs/src/nsContentPrefService.js
> > 
> > >     var currentValue = this.getPref(aURI, aName);
> > >     if (typeof currentValue != "undefined" && currentValue == aValue)
> > >       return;
> > > 
> > >     var settingID = this._selectSettingID(aName) || this._insertSetting(aName);
> > >     var group, groupID, prefID;
> > >     if (aURI) {
> > >       group = this.grouper.group(aURI);
> > >+      if (!group)
> > >+        return;
> > 
> > I wonder if this should throw to indicate that it couldn't set the preference. 
> > That would complicate the calling code but make the method's behavior less
> > opaque.
> 
> Could make sense, although I'd prefer to not change the interface.

This interface is pretty young and not yet used much, so the sooner the better if this is the right thing to do, as these kinds of changes will only get harder to make.  The question is just whether this is really the right thing to do.  Not sure.


> > >+    switch (aURI.scheme) {
> > >+      case "data":
> > >+      case "javascript":
> > >+        // Empty group, prefs won't be stored.
> > >+        return "";
> > >+    }
> > >+
> > 
> > Seems like this method should return null or undefined rather than the empty
> > string if the URI couldn't be grouped by hostname because there isn't a
> > hostname in the URI (either that or it should throw NS_ERROR_NOT_AVAILABLE).
> 
> "" instead of null is a bit of a hack, as nsIContentURIGrouper::group claims to
> return a string.

Looks like you changed this, which seems like the right thing to do.  Make sure you also update the comment in the IDL file to reflect this change.


> > Also, I wonder if it makes more sense to whitelist schemes that we know provide
> > hostnames (i.e. http, https) instead of blacklisting schemes that we know don't
> > provide them.
> 
> That might be too restrictive. I don't know all the use cases for content
> preferences.

Ok, fair enough, let's keep it this way for now.

Note: I suspect we may want to do something different for ftp: URLs as well at some point, since the FTP view is essentially chrome, and its text size doesn't get set by the servers, so it doesn't make much sense to let users set it per-site.


>Index: browser/base/content/browser-textZoom.js

>       if (typeof aValue != "undefined")
>         ZoomManager.fullZoom = this._ensureValid(aValue);
>-      else if (typeof this.globalValue != "undefined")
>-        ZoomManager.fullZoom = this.globalValue;
>-      else
>-        ZoomManager.reset();
>+      else if (aFromRequest != false) {
>+        if (typeof this.globalValue != "undefined")
>+          ZoomManager.fullZoom = this.globalValue;
>+        else
>+          ZoomManager.reset();
>+      }

Nit: given that onLocationChange in browser.js passes !!request, |aFromRequest != false| can just be |aFromRequest|.

But this isn't going to work, because we often set the zoom for the first time on tab switch, when there isn't a request, but when we do want to set the zoom to a global value or reset it to the default value if there isn't a site-specific value.

I think what we really want here is to stop setting the zoom on location change and start setting it on some other event that fires for each top-level document that loads in a tab (once the document starts to load, but before the initial reflow).

In the meantime, there's no ideal solution, but making this conditional be |(aURI.scheme != "data" && aURI.scheme != "javascript") || aFromRequest| might be the next best thing, as it:

1. Sets the zoom to the site-specific, global, or default value for URIs with hostnames on both page load and tab switch.
2. Sets the zoom to the global or default value for data: and javascript: URLs only on page load.

Which leaves only the bug that loading a data: or javascript: URL into a tab that isn't the current tab and then switching to it doesn't set the zoom for that tab to the global or default value.


>Index: toolkit/components/contentprefs/src/nsContentPrefService.js

>   getPref: function ContentPrefService_getPref(aURI, aName) {
>     if (aURI) {
>-      var group = this.grouper.group(aURI);
>+      var group;
>+      try {
>+        group = this.grouper.group(aURI);
>+      } catch (e) {}
>       return this._selectPref(group, aName);
>     }
...
>   _selectPref: function ContentPrefService__selectPref(aGroup, aSetting) {
>+    if (!aGroup)
>+      return undefined;
>+

Nit: this would be simpler if the catch clause returned the undefined value, since then you wouldn't have to modify _selectPref and its interface at all.

Comment 15

[Dão Gottwald [:dao]]

Not working on this anymore. I think the last patch wasn't too far off, but now it needs a serious update (e.g. ContentPrefSink has been removed).

Comment 16

[:Gijs (he/him)]

Attachment: Bug 385559 - stop putting full data URIs into the content preference service, r?#firefox-desktop-core-reviewers

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f991fbbff385
stop putting full data URIs into the content preference service, r=firefox-desktop-core-reviewers ,mossop

Comment 18

[Atila Butkovits]

Backed out for causing failures at test_DownloadLastDir_basics.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d59f7599c4104a2856a9da7f44b6318f8ef98cca

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=G9IVqbSKSqyC8-JXEDczNA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f991fbbff385e96078a84ceca5d4027ecd834a8d

Failure log: https://treeherder.mozilla.org/logviewer?job_id=492004944&repo=autoland&lineNumber=5842

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3d49de4cd37
stop putting full data URIs into the content preference service, r=firefox-desktop-core-reviewers ,mossop

Comment 20

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a3d49de4cd37