70219 - nsIMarkupDocumentViewer::zoom should be exposed through nsIDOMWindow

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Judson Valeski]

nsIMarkupDocumentViewer::zoom is private. This needs to be exposed through
nsIDOMWindow. nsIDOMWindow::Zoom should just call down through, probably,
nsIDocShell's zoom method.

Comment 1

[Judson Valeski]

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

Comment 2

[Judson Valeski]

requesting 0.9 milestone

Comment 3

[Aaron Leventhal]

Jud, this isn't going to make it into .9. I'm downgrading it because there is a 
font prefs API.

I assume you want this to take the place of the 
proposed nsIWebBrowserAccessibility (bug 71737).

Comment 4

[Judson Valeski]

ahh. if you don't need per window zoom capability, let's just invalidate this 
bug. I filed this because I thought you said we needed this on a per window 
basis?

Comment 5

[Judson Valeski]

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

Comment 6

[Aaron Leventhal]

Yes, I would prefer to have it exposed on a per-window basis, but is it really 
okay to add methods to nsIDOMWindow?

Comment 7

[Michael Dunn]

Moving API bug ownership to David Epstein.

Comment 8

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 9

[Judson Valeski]

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

Comment 10

[Aaron Leventhal]

Jud, is it still okay to add this method to nsIDOMWindow?

Comment 11

[Judson Valeski]

sure is.

Comment 12

[Aaron Leventhal]

Attachment: Patch exposes nsIMarkupDocument::textZoom through nsIDOMWindow::zoom

Comment 13

[Johnny Stenback (:jst)]

Sure, from an API frozen-ness point of view it's fine to change
nsIDOMWindowInternal, but why would we need this? I'm sure this will break
existing pages since this will predefine a global zoom variable in all scripts
out there, so anyone that uses a variable named zoom and doesn't declare it will
end up either changing the font sizes, or get an exception thrown in their face
if they're setting zoom to a value that's not convertable to a float. I don't
like this, do we actually have a reason for doing this? Does this need to be
scriptable?

Comment 14

[Aaron Leventhal]

Johhny,

It doesn't need to be scriptable, but it's for embeddors. Is there a better place?

We don't need to do zoom as an attribute, either. I would be fine with getZoom()
and setZoom()

Comment 15

[Johnny Stenback (:jst)]

Hmm, ok, I'm fine with adding this as a [noscript] attribute, but could we
rename this to |textZoom| or something less generic than just |zoom|?

Comment 16

[Aaron Leventhal]

Attachment: With Johnny's improvements

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 64999 [details] [diff] [review]
With Johnny's improvements

>            attribute DOMString                          name;
>+ /**
>+  * Set/Get the document scale factor.  When setting this attribute, a
>+  * NS_ERROR_NOT_IMPLEMENTED error may be returned by implementations
>+  * not supporting zoom.  Implementations not supporting zoom should return
>+  * 1.0 all the time for the Get operation.  1.0 is equals normal size, i.e. no zoom.
>+  */
>+  [noscript] attribute float textZoom;
>+
>   readonly attribute long                               scrollX;
>   readonly attribute long                               scrollY;
> 

Please push |textZoom| out to line up with the rest of the attributes.

>+    if (markupViewer)
>+      return markupViewer->GetTextZoom(aZoom);

Add braces around this (both places), even if it's only one line, just like the
rest (or most at least) of the nsGlobalWindow.cpp code does.

With that, sr=jst

Comment 18

[Aaron Leventhal]

Okay, done. Now looking for r=

Comment 19

[David Epstein]

changed qa contact to Ashish. nsIDomWindow is his area.

Comment 20

[Aaron Leventhal]

checked in

Comment 21

[Ashish Bhatt]

marking as verified