Status

enhancement
P2
normal
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: christian.bugzilla, Assigned: blassey)

Tracking

(Depends on 1 bug, {dev-doc-complete})

Dependency tree / graph
Bug Flags:
wanted-fennec1.0 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 9 obsolete attachments)

12.94 KB, patch
Details | Diff | Splinter Review
6.13 KB, patch
Details | Diff | Splinter Review
4.91 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
 
Flags: wanted-fennec1.0+
Priority: -- → P2
Assignee: nobody → pavlov
Assignee: pavlov → bholley
Status: NEW → ASSIGNED
attached are two patches (one content, one chrome) that together implement support for the viewport <META> tag. Flagging stuart and jst for review.

There are still problems as relating to bug 443076. Currently, the viewport of a page is always applied to the _next_ page loaded because of some hackiness in the view manager visibility code, so you more or less need to refresh each page to get the proper layout. A workaround for that bug that lets you see the proper viewport behavior is on the bug page.
Depends on: 443076
Comment on attachment 327879 [details] [diff] [review]
fennec chrome patch for viewport META support

This looks pretty good, just a few nits...

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml

>+        <xul:stack style="-moz-stack-sizing: ignore">

Shouldn't -moz-stack-sizing go on the hbox or browser? I wouldn't expect this to work - does it actually have an affect?

>+            <xul:browser anonid="browser"
>+                         flex="0"

Just omit the flex attribute?

>     <implementation>

>+      <!-- The zoom level. We make this a property so that we can clamp it
>+      between the maximimum and minimum zoom values and ignore new values if
>+      the document dissalows scaling. -->

typo: maximum, disallows

>+      <property name="_zoomLevel">
>+        <getter>
>+          return this._zoomLevelInternal;
>+        </getter>

Can just use an onget attribute for this.

>+        <setter>
>+          if (this._zoomEnabled) {
>+            var result = Math.max(Math.min(parseFloat(val), this._zoomMax),
>+                                  this._zoomMin);
>+            this._zoomLevelInternal = result;
>+          }
>+        </setter>

having conditionals like this in setters is confusing... moving the "if (this._zoomEnabled)" logic out to the callers would be clearer. Also, setters should return the value being set.

>+      <property name="_zoomFactor" readonly="true">

>+          if (bWidth > 0)
>+            return (w / bWidth) * this._zoomLevel;
>+          else
>+            return this._zoomLevel;

remove the else after return.

>+      <method name="_updateViewState">

>+          /* Reset the pan data. */

can you use // comments to match the rest of the file?

>+          /* Enable scaling. */
>+          this._zoomEnabled = 1;

>+          var vpUserScalable = 
>+            windowUtils.getDocumentMetadata("viewport-user-scalable");

>+          /* If the document disallows zooming, disable it. */
>+          if (vpUserScalable == "no")
>+            this._zoomEnabled = 0;

Just assign directly to _zoomEnabled (and make it a bool):

this._zoomEnabled = (vpUserScalable != "no");
Attachment #327879 - Flags: review?
Attachment #327881 - Flags: review? → review?(jst)
Attachment #327879 - Attachment is obsolete: true
Attachment #328072 - Flags: review?
Attachment #328072 - Flags: review? → review?(gavin.sharp)
Comment on attachment 327881 [details] [diff] [review]
content patch for viewport META support

- In ProcessViewportToken():

+  if (nsGkAtoms::height->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_height, value);
+  else if (nsGkAtoms::width->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_width, value);
+  else if (nsGkAtoms::initial_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_initial_scale, value);
+  else if (nsGkAtoms::minimum_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_minimum_scale, value);
+  else if (nsGkAtoms::maximum_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_maximum_scale, value);
+  else if (nsGkAtoms::user_scalable->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_user_scalable, value);

This will do a string compare for each if check. I think you'd probably end up with faster code if you'd get the atom for the key up front and then relied on pointer compares in the if check to find out which header to set etc.

- In nsContentUtils::ProcessViewportInfo():

+    /* Advance tip past non-separator characters. */
+    while (!IS_SEPARATOR(*tip) && (tip != end))
+      ++tip;

If we the ++tip increment puts us at the end of the string then the !IS_SEPARATOR(*tip) check will look past the end of the string (which may or may not be ok, depending on the actual string passed in.

+
+    /* Our token consists of the characters between tail and tip. */
+    ProcessViewportToken(aDocument, Substring(tail, tip));
+
+    /* Skip separators. */
+    while (IS_SEPARATOR(*tip) && (tip != end))

Same here.

r=jst with that fixed.
Attachment #327881 - Flags: review?(jst) → review+
Comment on attachment 328582 [details] [diff] [review]
revised content patch to address jst's suggestions

flagging jonas for sr
Attachment #328582 - Flags: superreview?(jonas)
Attachment #328582 - Flags: superreview?(jonas) → superreview+
Posted patch updated patch (obsolete) — Splinter Review
Updated this patch to apply and made a few fixes. This is going to need to be merged with my patch for bug 436059 once that lands.
Attachment #328072 - Attachment is obsolete: true
Attachment #328072 - Flags: review?(gavin.sharp)
Attachment #329136 - Flags: review?(gavin.sharp)
Target Milestone: Fennec M5 → Fennec M6
Requesting checkin for the content-patch and mochitests patch. The current content patch consists of trivial updates to the SR-ed-but-obsoleted content patch, and I don't think mochitests need review. 
Keywords: checkin-needed
Depends on: 445745
This broke compiling with MathML disabled, which is the default for Thunderbird.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGkAtomList.h#1318 is #ifdef MOZ_MATHML while http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4244 isn't, so we end up breaking in nsContentUtils.cpp:4244 with "error: ‘maximum_scale’ is not a member of ‘nsGkAtoms’"
(In reply to comment #14)
> This broke compiling with MathML disabled
That's bug 445745, which has now been fixed.
Target Milestone: Fennec M6 → Fennec M7
moving to a1 :/  still waiting on 443076
Target Milestone: Fennec M7 → Fennec A1
Target Milestone: Fennec A1 → Fennec A2
a1 -> a2? Sigh. To think this was once slated for M3.
I don't understand follow the comment "This information is ignored by the generic content sink because it only stores http-equiv meta tags."

Why isn't it appropriate to put the new ProcessMETATag() bits into nsContentSink instead of duplicating it in both XML and HTML content sinks?

(I'm trying to understand this in order to avoid breaking stuff in the HTML5 parser.)
Wow, it's been a while.

The comment refers to the fact that when the generic content sink does meta tag parsing, it only stores tags that are "http-equiv" flavor and not "name" flavor. This is why we have to do separate processing for them.

I don't remember why they're not combined into the superclass method. I think it may have to do with the fact that originally I was doing different stuff based on namespacing, but it then turned out that the behavior for xml and html was actually identical for this task. Assuming the information wouldn't screw anything up for other content sinks, I can't think of any reason (at the moment anyway) why they couldn't be factored out.
OK. I'm proceeding by hoisting the viewport processing into nsContentSink so that I don't need yet another copy for HTML5. Thanks.
Comment on attachment 329136 [details] [diff] [review]
updated patch

This has bitrotted quite a bit due to the canvasbrowser reworking.
bholley, are you going to be able to update it?
Attachment #329136 - Attachment is obsolete: true
Attachment #329136 - Flags: review?(gavin.sharp)
*shrug* - I could, though I don't really want to. I'm not working in mobile anymore, so it doesn't make a huge amount of sense for me to spend time figuring out the new canvas browser setup. I'd imagine it would take much less time for someone familiar with how things have progressed to port the patch.
Two websites useful for testing:

http://www.hopstop.com/pda - uses both Viewport and HandheldFriendly
http://m.precentral.net/ - only uses viewport

blassey said this bug was on his radar
Posted patch mobile patch (obsolete) — Splinter Review
Attachment #400792 - Flags: review?(combee)
Comment on attachment 400792 [details] [diff] [review]
mobile patch

If a website does not have the "viewport" metadata, we still do some work. Can you wrap the viewport math in a check for valid viewport strings?
Posted patch mobile patch v.2 (obsolete) — Splinter Review
addresses gavin and mfinkle's comments
Assignee: bobbyholley → bugmail
Attachment #400792 - Attachment is obsolete: true
Attachment #400866 - Flags: review?(mark.finkle)
Attachment #400792 - Flags: review?(combee)
Attachment #400866 - Flags: review?(mark.finkle) → review+
Comment on attachment 400866 [details] [diff] [review]
mobile patch v.2

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>+    let vpWidthStr = windowUtils.getDocumentMetadata("viewport-width"); 
>+    let vpHeightStr = windowUtils.getDocumentMetadata("viewport-height");
>+    let vpInitialScale = parseFloat(windowUtils.getDocumentMetadata("viewport-initial-scale"));
>+    let vpUserScalable = windowUtils.getDocumentMetadata("viewport-user-scalable");

Add a line break here
It be cool if you could just use viewportW and viewportH (even as the string versions) and then just reuse them when you parseInt below

And basically "vp" -> "viewport"

>+    let [w, h] = BrowserView.Util.getBrowserDimensions(browser);

browserW and browserH please

>+    var sw = window.screen.width;
>+    var sh = window.screen.height;
>+    var aspectRatio = sw / sh;

screenW and screenH please

>+    let newZoomLevel = BrowserView.Util.pageZoomLevel(this.getVisibleRect(), w, h);
>+    let newWidth = w;
>+    let newHeight = h;
>+    let newTextZoom =  browser.markupDocumentViewer.textZoom;

Do we want to touch textZoom at all?

Also, since I'm being so annal about this: newWidth -> newW and newHeight -> newH ?

>       delete  browser.handheld;
>-      let [w, h] = BrowserView.Util.getBrowserDimensions(browser);
>-      this.setZoomLevel(BrowserView.Util.pageZoomLevel(this.getVisibleRect(), w, h));
>     }
>+    if (vpWidthStr != "" && vpHeightStr !="") {
>+      let vpWidth, vpHeight;
>+      if (vpWidthStr == "device-width")

Add line break before the "if" block

>+      if ((vpWidth > 0) && (vpHeight > 0)) {
>+        browser.vpWidth = newWidth = vpWidth;
>+        browser.vpHeight = newHeight = vpHeight;

Please use browser.viewportWidth and browser.viewportHeight to be a little consistent with the other xxxWidth and xxxHeight properties.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-          browserStyle.height = ((browsers[i].hasOwnProperty("handheld") && 
>-                                  browsers[i].handheld) ? 
>-                                 scaledScreenH : scaledDefaultH) + "px";
>+	  let newHeight = scaledDefaultH;

Same annal naming here


Mostly all naming nits (sorry), except for the textZoom thing. Do we want to use it?

r+ with that answered
Attachment #400866 - Flags: review+ → review-
Comment on attachment 400866 [details] [diff] [review]
mobile patch v.2

this code needs to live somewhere other than zoomToPage.  probably needs to live off of one of the networkStart notifications, if we think we'd have this data at that point.
Posted patch patch v.3 (obsolete) — Splinter Review
Attachment #400866 - Attachment is obsolete: true
Attachment #408814 - Flags: review?(mark.finkle)
Posted patch Updated to trunk (obsolete) — Splinter Review
Updated to trunk with some changes to Util.contentIsHandheld
Attachment #408814 - Attachment is obsolete: true
Attachment #409989 - Flags: review?(gavin.sharp)
Attachment #408814 - Flags: review?(mark.finkle)
Attachment #409989 - Flags: review?(bugmail)
Comment on attachment 409989 [details] [diff] [review]
Updated to trunk

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>   getZoomForPage: function getZoomForPage() {

>+    let metaData = Util.contentIsHandheld(browser);
>+    if (metaData.reason == "handheld")
>       return 1;
>+    else if (metaData.reason == "viewport")
>+      return parseFloat(metaData.scale);

shouldn't we do some form of validation here? Seems like bad things will happen if this returns NaN.

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js

>   contentIsHandheld: function contentIsHandheld(browser) {
>     let doctype = browser.contentDocument.doctype;
>     if (doctype && /(WAP|WML|Mobile)/.test(doctype.publicId))
>-      return true;
>+      return {reason: "doctype", result: true};

no one seems to check for reason == "doctype", doesn't that regress bug 524900?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   endLoading: function() {

>+      let validW = !isNaN(viewportW) && viewportW > 0;
>+      let validH = !isNaN(viewportH) && viewportH > 0;

isNaN checks are redundant, NaN > 0 is false.

>+      } else if(!validW && validH){

nit: space after "if", before "{"
Attachment #409989 - Flags: review?(gavin.sharp) → review-
Comment on attachment 409989 [details] [diff] [review]
Updated to trunk

eturn {reason: "handheld", result: true};
> 
>-    return false;
>+    let viewportScale = windowUtils.getDocumentMetadata("viewport-initial-scale");
>+    if (viewportScale != "") {
>+      return {

changing this to:
    let viewportScale = parseFloat(windowUtils.getDocumentMetadata("viewport-initial-scale"));
    if (viewportScale > 0)

should alleviate Gavin's concern. (obviously you wouldn't re-parse also)
Updated based on review comments
Attachment #409989 - Attachment is obsolete: true
Attachment #410009 - Flags: review?(gavin.sharp)
Attachment #409989 - Flags: review?(bugmail)
Comment on attachment 410009 [details] [diff] [review]
updated for comment

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   endLoading: function() {

>+      } else if (!validW && validH){

still missing a space here, before "{" :)
Attachment #410009 - Flags: review?(gavin.sharp) → review+
pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/a212232f29a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: A2 → B5
any tips for how to verify this bug?
go to touch.facebook.com, you'll notice that it lays out 320px wide (unfortunately), which is the width specified by its viewport meta tag.
verified with 20091106 1.9.2 nightly build on n810
Status: RESOLVED → VERIFIED
Mdc doesn't really have documentation:
https://developer.mozilla.org/Firefox_Mobile_for_Developers#Viewport_meta_tag

I saw there were some mochitests for this bug, so I guess we can mark in-testsuite+ for this or do we need more tests?
Flags: in-testsuite?
Flags: in-litmus?
Keywords: dev-doc-needed
Depends on: 561202
Depends on: 561203
automated test candidate, not manual.  in-litmus-
Flags: in-litmus? → in-litmus-
I thought we had tests for this:
http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_viewport.js

Could we verify this and if not, please add specifics of what we need to get automated coverage for?
We have automated tests in fennec
Flags: in-testsuite? → in-testsuite+
The Fennec Emulator seems to not take in count the meta viewport feature, so i can't have my little HTML5 game working on the Fennec emulator : https://github.com/nfroidure/CasseBriques
Nicolas, what emulator are you referring to?
The program available here, i run it under Debian GNU/Linux : http://ftp.mozilla.org/pub/mozilla.org/mobile/releases/4.0.1/linux-i686/fennec-4.0.1.en-US.linux-i686.tar.bz2

I realize it's not really an emulator. It would be great to give a way to override my desktop screen size.

Is it planned ?
We stopped making desktop builds of fennec a while back (that build is 13 months old). You may want to run fennec in the ARM android emulator included in their sdk [http://developer.android.com/sdk/index.html].

Just to warn you, it will be sluggish. However, once bug 750366 is fixed, you'll be able to run fennec in the x86 android emulator, which is actually quite fast. You can do that today if you don't mind producing your own build.
You need to log in before you can comment on or make changes to this bug.