Closed Bug 737274 Opened 12 years ago Closed 11 years ago

Tab.setResolution should set the resolution of the contentWindow

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-fennec1.0 -)

RESOLVED FIXED
Firefox 28
Tracking Status
blocking-fennec1.0 --- -

People

(Reporter: blassey, Assigned: kats)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
      No description provided.
Attachment #607386 - Flags: review?(mark.finkle)
Attachment #607386 - Flags: review?(bugmail.mozilla)
Blocks: 737280
Comment on attachment 607386 [details] [diff] [review]
patch

>-      if (BrowserApp.selectedTab == this) {
>-        let cwu = window.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>-        cwu.setResolution(aZoom, aZoom);
>-      }
>+      let win = this.browser.contentWindow;
>+      let cwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+      cwu.setResolution(aZoom, aZoom);

Using the content window, not the XUL window, does seem to be the "right" thing to do and matches what we did in XUL fennec. You dropped the "if this is the selected tab" check though. Do we want to set this regardless? It shouldn't hurt and we also do that in XUL fennec.
Attachment #607386 - Flags: review?(mark.finkle) → review+
this is where the code went from setting resolution on the content document to setting it on the top level window:
https://hg.mozilla.org/mozilla-central/rev/726cd11889e6
Comment on attachment 607386 [details] [diff] [review]
patch

Review of attachment 607386 [details] [diff] [review]:
-----------------------------------------------------------------

I wish it were this easy. It doesn't work though, same as last time I tried it. If you build with this patch and pinch zoom, you'll see things scale up but that's only the compositor scaling what Gecko has drawn at a 1.0 resolution. Gecko doesn't actually draw the content at the resolution we give it.
Attachment #607386 - Flags: review?(bugmail.mozilla) → review-
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Using the content window, not the XUL window, does seem to be the "right"
> thing to do and matches what we did in XUL fennec.

It is the "right" thing to do, but as far as I understand, it's not what XUL fennec did. XUL fennec did this:

        let rootCwu = content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
        if (json.id == 1) {
          rootCwu.setResolution(json.scale, json.scale);

where "content" is AIUI the top-level XUL window.

(In reply to Brad Lassey [:blassey] from comment #2)
> this is where the code went from setting resolution on the content document
> to setting it on the top level window:
> https://hg.mozilla.org/mozilla-central/rev/726cd11889e6

That change just moves around the line of code in question; both before and after that cset we're setting the resolution on the top-level XUL window.
(In reply to Kartikaya Gupta (:kats) from comment #4)
> (In reply to Mark Finkle (:mfinkle) from comment #1)
> > Using the content window, not the XUL window, does seem to be the "right"
> > thing to do and matches what we did in XUL fennec.
> 
> It is the "right" thing to do, but as far as I understand, it's not what XUL
> fennec did. XUL fennec did this:
> 
>         let rootCwu =
> content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.
> nsIDOMWindowUtils);
>         if (json.id == 1) {
>           rootCwu.setResolution(json.scale, json.scale);
> 
> where "content" is AIUI the top-level XUL window.

Nope. In XUL Fennec (which was multi-process), "content" referred to the contentWindow of the Tab.
blocking-fennec1.0: --- → ?
blocks two beta bugs : 732089 732971
Blocks: 732016
blocking-fennec1.0: ? → beta+
Assignee: nobody → blassey.bugs
tn thinks we can fix bug 732016 without this
No longer blocks: 732016
Re-noming to minus - 732016 can be fixed without this.
Blocks: 732016
blocking-fennec1.0: beta+ → ?
blocking-fennec1.0: ? → -
Assignee: blassey.bugs → bugmail.mozilla
This was fixed as part of bug 732971.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: