218302 - text zoom should not be remembered when changing sites

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

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

Text zoom should not be remembered when changing from one web site to another. 
It currently is remembered when the two sites are viewed in the same window, but
it should not be.

This has been discussed at great length in bug 65571.  Bug 108391, for
remembering the settings for each "site" (however we define it), depends on this
bug.

Steps to reproduce:
 * Load this bug
 * Press Ctrl-+ a few times
 * load http://www.nytimes.com/ in the same window.

Actual results:
 * the fonts are abnormally large

Expected results:
 * the fonts should be normal size

Comment 1

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

*** Bug 217706 has been marked as a duplicate of this bug. ***

Comment 2

[Felix Miata]

Zoom isn't remembered in other tabs or other windows or across browser restarts.
Can't we keep this one option of having zoom stick from site to site? Do we
really need to make zoom so unsticky?

Comment 3

[fantasai]

Attachment: patch

I'm not certain of the comments I added, but the code works. :)

Comment 4

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

Comment on attachment 164279 [details] [diff] [review]
patch

>         else {
>             // No old content viewer, so get state from parent's content viewer
>             nsCOMPtr<nsIContentViewer> parentContentViewer;
>             parent->GetContentViewer(getter_AddRefs(parentContentViewer));
>             oldMUDV = do_QueryInterface(parentContentViewer);
>+            oldDocViewer = do_QueryInterface(parentContentViewer);
>         }

I think this case has something to do with frames or iframes.  So this seems
like you're going to break text zoom applying to some of them when there are
frames or iframes cross-site.  I suspect you probably want to consider this
case as sameHost unconditionally, rather than checking hosts.  (Which probably
means renaming the variable. :-)

>+                    nsIDocument* doc;
>+                    rv = newDocViewer->GetDocument(&doc);

This |AddRefs|, so you should use nsCOMPtr and getter_AddRefs.	(Your current
code leaks.)

>+                    newURI = doc->GetDocumentURI();

Just declare |newURI| here, rather than above.	(The general guideline for C++
(rather than C) is to declare variables as late as possible.)



I'm also wondering if this is really the best way to compare hosts.  There
might be existing functions you could use, or some way to simplify the code
you've written.  And if there aren't existing functions for what you want, you
still might want to put it somewhere else since it seems like something that
might be useful elsewhere, although maybe all the things that it would be
useful for are in this function.  darin or bz might have some ideas.

Comment 5

[Darin Fisher]

Comment on attachment 164279 [details] [diff] [review]
patch

>Index: mozilla/docshell/base/nsDocShell.cpp

>+                    nsCAutoString newHost;
...
>+                    if (doc) {
>+                        newURI->GetHost(newHost);

I think you can move the declaration of newHost down to here, where it
is actually used.

>+                        if (!newHost.IsEmpty()) {
>+                            nsCAutoString oldHost;
>+                            doc->GetDocumentURI()->GetHost(oldHost);
>+                            if (oldHost.Equals(newHost)) {
>+                                sameHost = PR_TRUE;

This is probably the best way to assert that the hostnames are the same,
but perhaps it would be better to compare "scheme://host[:port]" instead,
and what about two hosts that are in the same domain?

In the case where there is no hostname, I think you should check that the
schemes are the same.  I don't think it makes sense to check for file://
explicitly.

Comment 6

[fantasai]

Attachment: new patch

If we check for port and protocol when there is a hostname, then we'll break in
changes between http and https.

Comment 7

[Scott MacGregor]

Comment on attachment 170012 [details] [diff] [review]
new patch

Darin has already been commenting on this nsDocShell change. I'm guessing he's
better suited to providing the sr.

Comment 8

[Darin Fisher]

Comment on attachment 170012 [details] [diff] [review]
new patch

>Index: mozilla/docshell/base/nsDocShell.cpp

>+        if (parent) {
>+           //keep zoom and styleDisabled settings steady within a child frame
>+           //so parent and child always match
>+           sameHost = PR_TRUE;
>+        }

nit: please add a space between "//" and start of text.


>+                    nsCOMPtr<nsIDocument> doc;
>+                    rv = newDocViewer->GetDocument(getter_AddRefs(doc));
>+                    if (NS_FAILED(rv)) return rv;
>+                    if (doc) {
>+                       nsIURI* newURI = doc->GetDocumentURI();
>+
>+                       rv = oldDocViewer->GetDocument(getter_AddRefs(doc));
>+                       if (NS_FAILED(rv)) return rv;
>+                       if (doc) {

so, in reality this guy pretty much never fails.  do we
really care to propagate the exception in that case?  can
we just live with |if (doc)|?


>+                          nsIURI* oldURI = doc->GetDocumentURI();
>+
>+                          nsCAutoString newHost;
>+                          newURI->GetHost(newHost);
>+                          nsCAutoString oldHost;
>+                          oldURI->GetHost(oldHost);
>+
>+                          if (oldHost.Equals(newHost)) {
>+                             if (oldHost.IsEmpty()) {
>+                                nsCAutoString scheme;
>+                                oldURI->GetScheme(scheme);
>+                                newURI->SchemeIs(scheme.get(), &sameHost);
>+                             }
>+                             else {
>+                                sameHost = PR_TRUE;
>+                             }

what about the port numbers?  are you basically trying to
check if these are the same "scheme://host:port"?  if so,
then maybe it would be better to test that.

also, how about moving this checking code into a subroutine.
i think this function is already too long.

static PRBool IsSameHost(nsIURI* a, nsIURI* b);

Comment 9

[Darin Fisher]

Comment on attachment 170012 [details] [diff] [review]
new patch

minusing to get attention ;-)

Comment 10

[fantasai]

Attachment: patch, taking Darin's comments into account

Nah, I heard you. I'm just kinda slow. :)

I'm not checking port numbers because a switch from http to https shouldn't
trigger a mismatch.

Comment 11

[Darin Fisher]

Comment on attachment 177643 [details] [diff] [review]
patch, taking Darin's comments into account

>Index: docshell/base/nsDocShell.cpp

>+static inline PRBool
>+IsSameHost(nsIDocumentViewer* aDocViewerA,
>+           nsIDocumentViewer* aDocViewerB)
>+{
>+   nsCOMPtr<nsIDocument> docA;
>+   aDocViewerA->GetDocument(getter_AddRefs(docA));

indentation in nsDocShell.cpp is 4 whitespaces.  please use that style :)


>+      nsIURI* uriA = docA->GetDocumentURI();
>+      nsIURI* uriB = docB->GetDocumentURI();
>+    
>+      nsCAutoString hostA;
>+      uriA->GetHost(hostA);
>+      nsCAutoString hostB;
>+      uriB->GetHost(hostB);

can GetDocumentURI() ever return NULL?	is it at all possible?


>+                    if (newDocViewer) {
>+                       sameHost = IsSameHost(oldDocViewer, newDocViewer);
>+                    }
>+                }

again, watch the indentation


sr=darin with these nits addressed

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm...  So with that patch a data: (or javascript, or wyciwyg) uri can't share
the text zoom with the original uri.

Is that desirable?  If not, perhaps we want to use the URI from the document
principal instead?

Comment 13

[fantasai]

Boris,
  Can you give an example of a case where this would be a problem? I don't think
I'm following you very well. And what's a "document principal"? With the patch as
it stands, embedded docs (like iframes) should use the same zoom level as their
parent. I would guess that that covers most relevant cases, no?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

<iframe src="data:text/html,<h2>This is a data: iframe</h2>">

Comment 15

[Darin Fisher]

Or, consider document.open() (which results in a wyciwyg URL) and javascript:
URLs.  Boris has a pretty good point.

Comment 16

[Boris Zbarsky [:bzbarsky]]

fantasai, consider an HTML page that has:

  <a href="data:text/plain,This is some text">Click here</a>

Does clicking that link preserve the zoom level?

The document principal is the document's security context (see GetPrincipal() on
nsIDocument and nsIPrincipal).  It happens to include a codebase URI, which is
_usually_ the document URI, but for things like wycywig and javascript: and such
it's hacked to be the URI of the opening document (or at least should be hacked;
we have some bugs in that)...

Comment 17

[fantasai]

Attachment: patch, taking bz's comments into account

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm not sure I can get to this anytime soon (sooner than 2 weeks, basically)....
 I'm totally swamped right now.  :(

Comment 19

[fantasai]

well, that minus the unintended whitespace change at the top

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177764 [details] [diff] [review]
patch, taking bz's comments into account

>Index: mozilla/docshell/base/nsDocShell.cpp
>+IsSameHost(nsIDocumentViewer* aDocViewerA,

>+                        PRBool isSame = PR_FALSE;
>+                        uriA->GetScheme(scheme);
>+                        uriB->SchemeIs(scheme.get(), &isSame);

You need to check the rv of both GetScheme and SchemeIs, since people can drop
in semi-bogus URI impls....

>@@ -5067,10 +5130,14 @@

>+        // Always set text zoom: need to set to 1.0 if not preserving, 'coz

"because", please.

>+        // otherwise the deviceContext will hang onto the old setting

"on to".

r+sr=bzbarsky with that.

Comment 21

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

With the checkin for bug 378549, this bug is now fixed.  You should see the
requested behavior in the latest Minefield nightly builds and in the upcoming
Gran Paradiso alpha 6 release.

Comment 22

[Steffen Wilberg]

Comment on attachment 177764 [details] [diff] [review]
patch, taking bz's comments into account

Obsoleting old patch which hasn't been checked in.