Closed Bug 746502 Opened 12 years ago Closed 12 years ago

Browser API: add support for <meta name=viewport>

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla18
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: benfrancis, Assigned: drs)

References

(Blocks 1 open bug)

Details

(Keywords: feature, verifyme, Whiteboard: [LOE:M] [WebAPI:P0])

Attachments

(1 file, 6 obsolete files)

Add support for <meta name=viewport> on B2G/async panning and zooming
51.36 KB, patch
drs
: review+
smaug
: review+
Details | Diff | Splinter Review 
Not sure where to file this as according to this wiki page https://developer.mozilla.org/en/Mobile/Viewport_meta_tag this is already supported on Firefox Mobile, but apparently it's something we need for B2G.

This bug covers the implementation of the de-facto standard of the viewport meta tag which allows web pages to be viewed at a readable physical size at different pixel densities.
Assignee: nobody → mculpepper
Pretty important feature for a mobile browser, let alone a mobile OS. I'm going to yoink this from Marshall if he doesn't mind.

Ho boy, our <meta name=viewport> code is "interesting". Fennec basically implements it on its own. Twice. [1,2]. *headdesk*

And then nsContentUtils implements it *again* [3] just for font scaling [4]. http://e.asset.soup.io/asset/3147/2254_580c.png

There are plans to consolidate this: bug 716575. I will see how far along these are. I fear that within the desired B2G timeframe, the best I can do is consolidate the various frontend pieces into *one* implementation that we use across XUL, Android, and B2G.


[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4398
[2] https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/content.js#689
[3] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4974
[4] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#4911
Assignee: marshall → philipp
Talked to :jwir3 and :mbrubeck. :jwir3 says that bug 716575 is about 50% done, but needs input from somebody who knows what else is missing. :mbrubeck said he can help.

Despite this, I think we should try to get something together from the existing front-end code, just to make sure we don't miss the B2G timeframe.

I think we basically want to apply the Fennec logic to all <iframe mozbrowser>s, right?
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> I think we basically want to apply the Fennec logic to all <iframe
> mozbrowser>s, right?

Right.  Note that the default viewport in Fennec (for documents without explicit meta tags) is width=980, for compatibility with desktop web sites.  Browser tabs in B2G should use the same default as Fennec, but you might want to change this for installed apps if you want them to default to width=device-width instead (as everything in Gaia currently does).
That sounds like a good plan, Matt.
Ok, moving this over to the Browser API chain of bugs. Nomming for blocking because it's arguably a pretty important feature of the mobile web.
Blocks: browser-api
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Component: General → DOM: Core & HTML
Product: Boot2Gecko → Core
Summary: Add support for <meta name="viewport"> → Browser API: add support for <meta name=viewport>
Version: unspecified → Trunk
This should not be part of the <browser> API.  Gecko should handle this internally, automagically.  (Think of this like a proto-CSS feature.)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> This should not be part of the <browser> API.  Gecko should handle this
> internally, automagically.  (Think of this like a proto-CSS feature.)

Yes, that's what bug 716575 is about. Timeline on that is somewhat unclear, while Fennec has existing code that we can lift to <iframe mozbrowser> for now. It's not an either/or question, just a short term vs long term problem.
The proto-CSS comment reminds me that http://dev.w3.org/csswg/css-device-adapt/ specifies both the meta tag and a CSS equivalent (which I believe is implemented by Opera).  We should aim at supporting both in Gecko, long-term.
> Timeline on that is somewhat unclear, while Fennec has existing code that we can lift to <iframe 
> mozbrowser> for now.

Can you give me some idea of how much code we're talking about here?
(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> The proto-CSS comment reminds me that
> http://dev.w3.org/csswg/css-device-adapt/ specifies both the meta tag and a
> CSS equivalent (which I believe is implemented by Opera).  We should aim at
> supporting both in Gecko, long-term.

Agreed. Do we have a bug for the CSS equivalent yet?

(In reply to Justin Lebar [:jlebar] from comment #9)
> Can you give me some idea of how much code we're talking about here?

AFAICT, ViewportHandler in https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4398
Tab.prototype.updateViewportMetadata and updateViewportSize also need to be included:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3034

All together it's around 250 lines of JavaScript.
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > This should not be part of the <browser> API.  Gecko should handle this
> > internally, automagically.  (Think of this like a proto-CSS feature.)
> 
> Yes, that's what bug 716575 is about. Timeline on that is somewhat unclear,
> while Fennec has existing code that we can lift to <iframe mozbrowser> for
> now. It's not an either/or question, just a short term vs long term problem.

Non sequitur.  I think we're confusing "implemented by 'frontend' code in gecko" and "part of browser API".
(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> Non sequitur.  I think we're confusing "implemented by 'frontend' code in
> gecko" and "part of browser API".

Erm, ok. So Fennec only applies it to its tabs browsers. AFAICT, the B2G equivalent would be <iframe mozbrowser>s. But I'll take a different spec.
> Non sequitur.  I think we're confusing "implemented by 'frontend' code in gecko" and "part of 
> browser API".

The only question is whether this is implemented in BrowserElement{Child,Parent}.js, versus somewhere else, in presumably C++ Gecko, right?  I understand that either way, this won't be part of the API exposed to pages which contain <iframe mozbrowser> (the "browser API").

> AFAICT, the B2G equivalent would be <iframe mozbrowser>s.

Yes.
(In reply to Justin Lebar [:jlebar] from comment #14)
> > Non sequitur.  I think we're confusing "implemented by 'frontend' code in gecko" and "part of 
> > browser API".
> 
> The only question is whether this is implemented in
> BrowserElement{Child,Parent}.js, versus somewhere else, in presumably C++
> Gecko, right?

That's indeed the implementation detail that I was talking about. cjones OTOH is talking about the policy whether to make <meta name=viewport> an inherent feature of <iframe mozbrowser> or just something we support for compatibility reasons in, say, the browser app. I'll start a thread on dev-b2g for the policy discussion.
> cjones OTOH is talking about the policy whether to make <meta name=viewport> an inherent feature of 
> <iframe mozbrowser> or just something we support for compatibility reasons in, say, the browser app.

I'm still not entirely clear what it means for <meta name=viewport> to be an "inherent feature of <iframe mozbrowser>."

Do you mean: Suppose I loaded a page in <iframe mozbrowser> in a desktop browser.  Should we respect <meta name=viewport> there?

Otherwise, is the question "should <meta name=viewport> be meaningful for apps?"
(In reply to Justin Lebar [:jlebar] from comment #16)
> Otherwise, is the question "should <meta name=viewport> be meaningful for
> apps?"

That's precisely the question.
(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> (In reply to Justin Lebar [:jlebar] from comment #16)
> > Otherwise, is the question "should <meta name=viewport> be meaningful for
> > apps?"
> 
> That's precisely the question.

Existing mobile content relies extensively on <meta name=viewport>.  While "width=device-width" is by far the most common setting, it's not too unusual for content to choose other values (like explicitly setting "width=320" or "width=600").    Supporting this tag in apps as well as the browser would help make it easier for existing web apps to look the same on B2G as the do on other platforms/browsers.
Thanks; the e-mail to the list makes it very clear.  But I expect whatever we decide will be simple to do, in terms of the implementation.
Supporting meta viewport in apps isn't something I want to do unless we have great pressure to do so.  It's a big UX footgun in general.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Supporting meta viewport in apps isn't something I want to do unless we have
> great pressure to do so.  It's a big UX footgun in general.

I would agree that it's a hack around the fact that mobile browsers originally had to deal with websites that weren't designed to recognize small device screens. And I agree that we should choose different defaults for bock standard web content (make that behave like Fennec) vs. installed apps (default to device-width). As far as supporting at all for apps, I can go either way really. See also dev-b2g thread.
per fennec peeps, to release without this ensures a mostly-broken mobile browser. -> blocking.
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
How are we doing here?
Priority: -- → P1
Nobody has cycles to work on this, at the moment.
philikon isn't currently working on this.  I'll try to get to it myself.
Assignee: philipp → nobody
Assignee: nobody → justin.lebar+bug
It's not clear to me what the decision was in that dev-b2g thread: Will we respect viewport for apps, or not?

This isn't a critical part of the implementation that I need to know upfront; it's a simple change to only support viewport in browser tabs, or to support viewport in both apps and browser tabs.
I'm not sure we reached a final conclusion. FWIW, my plan was to make it pref'able so that we can have a very low-impact change of minds later. (As in, something we'd feel comfortable changing in a beta release.)
Doug, this is at the same priority as respecting touch listeners in content, and should be in your wheelhouse to boot :).  Something to put on your list.
If someone else wants to take this bug, please be my guest!  Let me know if you're not going to take it so I can get to work.
Assignee: justin.lebar+bug → nobody
QA Contact: bugzilla
Assignee: nobody → bugzilla
QA Contact: bugzilla
FYI, bug 716575 is seeing some progress. Might be relevant.
Whiteboard: [LOE:S]
(In reply to Philipp von Weitershausen [:philikon] from comment #31)
> FYI, bug 716575 is seeing some progress. Might be relevant.

Not really relevant, but it's nice that we're unifying them to use the same thing (I'm working on using it too, but in C++ inside TabChild). I think this could be generalized more than the GetViewportInfo() function does already.
Attached patch WIP (obsolete) — — Splinter Review 
Mostly not working, posted for the purposes of collaboration
Depends on: 716575
Proposed patch. This provides support for meta viewport and fixes a few other things on the side. Of note:

1) Orientation changes should probably set the zoom such that the width of the visible section of the page on the screen remains the same. They will also properly repaint instead of waiting until the user pans/zooms.
2) Pages without a meta viewport will still be fitted to the screen better.
3) On first paint, we won't see scroll indicators anymore, and we will prerender more of the page (proper fix to bug 776413).

This patch relies heavily on unpushed changes in bug 716575 and bug 784908.

Need a second reviewer. smaug doesn't want to do it.
Attachment #654030 - Attachment is obsolete: true
Attachment #656769 - Flags: review?(jones.chris.g)
Follow-up work that will be needed:
1) When the orientation changes, it's possible that if we were already completely zoomed out, we zoom out further and go over the page bounds. This work is already required as part of bug 780395.
2) Double-tap-to-zoom doesn't take into account the min/max zooms set by the meta viewport. It should in the future.
Depends on: 784908
(In reply to Doug Sherk (:drs) (:dRdR) from comment #34)
>  smaug doesn't want to do it.
I didn't quite say that ;)
I'm just a bit weary doing reviews atm, so I won't be able to review this week.
No longer depends on: 784908
Depends on: 784908
(In reply to Olli Pettay [:smaug] from comment #36)
> (In reply to Doug Sherk (:drs) (:dRdR) from comment #34)
> >  smaug doesn't want to do it.
> I didn't quite say that ;)
> I'm just a bit weary doing reviews atm, so I won't be able to review this
> week.

Ah, ok, I misinterpreted :)

I think waiting a week is fine for this, we're depending on the work in bug 716575 which may take time to land.
Attachment #656769 - Flags: review?(bugs)
Rebased.
Attachment #656769 - Attachment is obsolete: true
Attachment #656769 - Flags: review?(jones.chris.g)
Attachment #656769 - Flags: review?(bugs)
Attachment #656861 - Flags: review?(jones.chris.g)
Attachment #656861 - Flags: review?(bugs)
@smaug: btw, when you get to this, I think it's fine if you just review the DOM stuff. I suspect a lot of this will be unfamiliar to you. If you want I can split it up, but I actually think it would make it much less clear what's going on.
Whiteboard: [LOE:S] → [LOE:]
Whiteboard: [LOE:] → [LOE:M]
Some questions before getting into details
 - what code is responsible for deciding what viewport size is chosen?  For example, what's responsible for deciding when to use the nytimes viewport (980px width)?  Is this in nsContentUtils or up to the client?  Is there a good comment about this stuff somewhere I can read?

 - it looks like we're remoting a lot more data through ViewportInfo than we need.  Instead of writing some more gross pickling code, let's write an IPDL struct that has what we need.

 - we should distance ourselves from <meta viewport>, since its functionality is moving into CSS proper.

Overall this looks like what I was expecting.  Nice work.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> Some questions before getting into details
>  - what code is responsible for deciding what viewport size is chosen?  For
> example, what's responsible for deciding when to use the nytimes viewport
> (980px width)?  Is this in nsContentUtils or up to the client?  Is there a
> good comment about this stuff somewhere I can read?

That happens in nsContentUtils::GetViewportInfo(). While we have a DEFAULT_BROWSER_(WIDTH|HEIGHT) in this patch, it basically doesn't do anything except for helping to decide initial scaling.

>  - it looks like we're remoting a lot more data through ViewportInfo than we
> need.  Instead of writing some more gross pickling code, let's write an IPDL
> struct that has what we need.

I actually don't agree with this as we're remoting things like FrameMetrics all the time which is a much bigger struct, and we've run into cases where we've added features that need new metrics that we previously didn't use. Thus in these cases it was easier to have the whole thing remoted already.

This also doesn't get passed around anywhere near as often as FrameMetrics. ViewportInfo is serialized and sent on page load, orientation flips, and browser chrome events (like opening up the awesome bar or bookmarks).

If we don't use ViewportInfo, I think the better option would be to just pass things directly in a function call, especially since we're only using 3 or so parameters at the moment.

>  - we should distance ourselves from <meta viewport>, since its
> functionality is moving into CSS proper.

Sure. As long as we have it tied to async browser content only I think we can easily move away from this.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
>  - it looks like we're remoting a lot more data through ViewportInfo than we
> need.  Instead of writing some more gross pickling code, let's write an IPDL
> struct that has what we need.

Ok, this patch changes is identical to the previous except that it passes around user-scalable, minimum-scale and maximum-scale instead of the ViewportInfo struct.
Attachment #656861 - Attachment is obsolete: true
Attachment #656861 - Flags: review?(jones.chris.g)
Attachment #656861 - Flags: review?(bugs)
Attachment #657280 - Flags: review?(jones.chris.g)
Attachment #657280 - Flags: review?(bugs)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #41)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> >  - we should distance ourselves from <meta viewport>, since its
> > functionality is moving into CSS proper.
> 
> Sure. As long as we have it tied to async browser content only I think we
> can easily move away from this.

I meant in the terminology we use.  "Custom viewport" might be better.
Comment on attachment 657280 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

This needs tests, at least for the easily testable parts.

>+NS_IMETHODIMP
>+TabChild::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("DOMMetaAdded")) {
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mWebNav->GetDocument(getter_AddRefs(domDoc));
>+    if (domDoc) {
>+      nsCOMPtr<nsIDocument> document(do_QueryInterface(domDoc));
>+      if (document) {
>+        UpdateMetaViewport();
>+      }
>+    }
>+  }
>+
>+  return NS_OK;
>+}
I don't understand this method?
It takes (or actually may even create) document, and if there is document calls UpdateMetaViewport


>+TabChild::OnLocationChange(nsIWebProgress* aWebProgress,
>+                           nsIRequest* aRequest,
>+                           nsIURI *location,
>+                           uint32_t aFlags)
>+{
>+  if (!IsAsyncPanZoomEnabled()) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMWindow> window;
>+  aWebProgress->GetDOMWindow(getter_AddRefs(window));
>+  if (!window) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMDocument> progressDoc;
>+  window->GetDocument(getter_AddRefs(progressDoc));
>+  if (!progressDoc) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  mWebNav->GetDocument(getter_AddRefs(domDoc));
>+  if (!domDoc || !SameCOMIdentity(domDoc, progressDoc)) {
>+    return NS_OK;
>+  }
You have quite a bit code duplication to get document.
Attachment #657280 - Flags: review?(bugs) → review-
How is <meta name=viewport> supposed to work in non-OOP frames?
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P0]
Comment on attachment 657280 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

Sorry for the review lag here.  I thought I'd reviewed this in Brazil.

For future reference, if this had been written as two patches, one
which moved the existing logic out of BrowserElementScrolling and into
TabChild, then a second which added viewport fudgery, the review would
have gone much more quickly.

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    /**
>+     * A new document has been loaded, and we have to parsed its meta viewport
>+     * tag.
>+     */
>+    UpdateMetaViewport(bool aAllowZoom, float aMinZoom, float aMaxZoom);
>+

This doesn't have anything specific to do with <meta viewport>.  This
is just zooming configuration.  Let's give it a more precise name,
like UpdateZoomParameters().  And propagate this naming change down
the list.

The comment here, while technically true, isn't really meaningful for
this interface.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+static const int32_t DEFAULT_BROWSER_WIDTH = 980;
>+static const int32_t DEFAULT_BROWSER_HEIGHT = 480;

Make this nsIntSize.  Naming style is |kDefaultViewportSize|.

>-nsresult
>+NS_IMETHODIMP
>+TabChild::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("DOMMetaAdded")) {
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mWebNav->GetDocument(getter_AddRefs(domDoc));
>+    if (domDoc) {
>+      nsCOMPtr<nsIDocument> document(do_QueryInterface(domDoc));
>+      if (document) {

I don't think there's any way we could get DOMMetaAdded without having
a document, and it's not used here.  So I would remove all this code.

>+NS_IMETHODIMP
> TabChild::Observe(nsISupports *aSubject,
>                   const char *aTopic,
>                   const PRUnichar *aData)

>+  } else if (!strcmp(aTopic, "before-first-paint")) {

DOMMetaAdded can arrive before this, right?  Why do we always reset
these values instead of initializing mLastMetrics to default values
and then going through the normal display-state update?

We should avoid jerking these values around, because in general
changing them can be *very* expensive.

>+void
>+TabChild::SetCSSViewport(float aWidth, float aHeight)
>+{

>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mWebNav->GetDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDocument> document(do_QueryInterface(domDoc));
>+

Unused?

>+void
>+TabChild::UpdateMetaViewport()

This method is another misnomer.  You're updating display state for
the window here.

>+  // This change to the zoom accounts for all types of changes I can conceive:
>+  // 1. screen size changes, CSS viewport does not (pages with no meta viewport
>+  //    or a fixed size viewport)
>+  // 2. screen size changes, CSS viewport also does (pages with a device-width
>+  //    viewport)
>+  // 3. screen size remains constant, but CSS viewport changes (meta viewport
>+  //    tag is added or removed)
>+  // 4. neither screen size nor CSS viewport changes
>+  //
>+  // In all of these cases, we maintain how much actual content is visible
>+  // within the screen width. Note that "actual content" may be different with
>+  // respect to CSS pixels because of the CSS viewport size changing.
>+  int32_t oldScreenWidth = mLastMetrics.mCompositionBounds.width;
>+  if (!oldScreenWidth) {
>+    oldScreenWidth = mInnerSize.width;
>+  }
>+  float zoomScale = (screenW * oldBrowserWidth) / (oldScreenWidth * viewportW);
>+

I don't understand what this calculation is doing.

>+  float zoom = clamped(double(mLastMetrics.mZoom.width * zoomScale),
>+                       viewportMetaData.minZoom, viewportMetaData.maxZoom);

It would make me happier if the last 30 lines of code or so were in
common code.  Is there a reason not to do that?  We can do this in a
followup.


>diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h

>+  // Stores the most up-to-date <meta name="viewport"> info.
>+  bool mAllowZoom;
>+  float mMinZoom;
>+  float mMaxZoom;
>+

This information doesn't necessarily come from <meta viewport>.  Just
describe what these variables actually are.
Attachment #657280 - Flags: review?(jones.chris.g)
Keywords: feature
(In reply to Chris Jones [:cjones] [:warhammer] from comment #46)
> It would make me happier if the last 30 lines of code or so were in
> common code.  Is there a reason not to do that?  We can do this in a
> followup.

Filed bug 793462.
Blocks: native-viewport
(In reply to Olli Pettay [:smaug] from comment #44)
> You have quite a bit code duplication to get document.

Not sure how I can effectively deal with this? There's some duplication that can be generalized. For example, I added a GetDOMWindowUtils() function to generalize getting the nsIDOMWindowUtils class. I don't see a way to do this with the nsIDOMDocument/nsIDocument code, though.

> This needs tests, at least for the easily testable parts.

Can you take a look at the tests in bug 716575 and tell me if you're happy with those? See attachment 654679 [details] [diff] [review].

(In reply to Chris Jones [:cjones] [:warhammer] from comment #46)
> DOMMetaAdded can arrive before this, right?  Why do we always reset
> these values instead of initializing mLastMetrics to default values
> and then going through the normal display-state update?
> 
> We should avoid jerking these values around, because in general
> changing them can be *very* expensive.

I'm wary of changing any of this stuff because it's almost identical, line-for-line, to Fennec's equivalent code in browser.js. When we generalize this in bug 793462, it will be advantageous to have them already almost identical, even if they're not completely correct until then.

> >+  // This change to the zoom accounts for all types of changes I can conceive:
> >+  // 1. screen size changes, CSS viewport does not (pages with no meta viewport
> >+  //    or a fixed size viewport)
> >+  // 2. screen size changes, CSS viewport also does (pages with a device-width
> >+  //    viewport)
> >+  // 3. screen size remains constant, but CSS viewport changes (meta viewport
> >+  //    tag is added or removed)
> >+  // 4. neither screen size nor CSS viewport changes
> >+  //
> >+  // In all of these cases, we maintain how much actual content is visible
> >+  // within the screen width. Note that "actual content" may be different with
> >+  // respect to CSS pixels because of the CSS viewport size changing.
> >+  int32_t oldScreenWidth = mLastMetrics.mCompositionBounds.width;
> >+  if (!oldScreenWidth) {
> >+    oldScreenWidth = mInnerSize.width;
> >+  }
> >+  float zoomScale = (screenW * oldBrowserWidth) / (oldScreenWidth * viewportW);
> >+
> 
> I don't understand what this calculation is doing.

That's ok, I don't really either, it's more browser.js code.
Attachment #657280 - Attachment is obsolete: true
Attachment #663789 - Flags: review?(jones.chris.g)
Attachment #663789 - Flags: review?(bugs)
Filed bug 793462 for some followup work.
Blocks: 793476
(In reply to Doug Sherk (:drs) (:dRdR) from comment #48)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #46)
> > DOMMetaAdded can arrive before this, right?  Why do we always reset
> > these values instead of initializing mLastMetrics to default values
> > and then going through the normal display-state update?
> > 
> > We should avoid jerking these values around, because in general
> > changing them can be *very* expensive.
> 
> I'm wary of changing any of this stuff because it's almost identical,
> line-for-line, to Fennec's equivalent code in browser.js. When we generalize
> this in bug 793462, it will be advantageous to have them already almost
> identical, even if they're not completely correct until then.
> 

That's not a very good argument :).  However, if we can't measure perf regressions from jerking the values around, then we have no hope of fixing them.

> > >+  // This change to the zoom accounts for all types of changes I can conceive:
> > >+  // 1. screen size changes, CSS viewport does not (pages with no meta viewport
> > >+  //    or a fixed size viewport)
> > >+  // 2. screen size changes, CSS viewport also does (pages with a device-width
> > >+  //    viewport)
> > >+  // 3. screen size remains constant, but CSS viewport changes (meta viewport
> > >+  //    tag is added or removed)
> > >+  // 4. neither screen size nor CSS viewport changes
> > >+  //
> > >+  // In all of these cases, we maintain how much actual content is visible
> > >+  // within the screen width. Note that "actual content" may be different with
> > >+  // respect to CSS pixels because of the CSS viewport size changing.
> > >+  int32_t oldScreenWidth = mLastMetrics.mCompositionBounds.width;
> > >+  if (!oldScreenWidth) {
> > >+    oldScreenWidth = mInnerSize.width;
> > >+  }
> > >+  float zoomScale = (screenW * oldBrowserWidth) / (oldScreenWidth * viewportW);
> > >+
> > 
> > I don't understand what this calculation is doing.
> 
> That's ok, I don't really either, it's more browser.js code.

...
(In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> ...

It's calculating what the new zoom should be if either the compositing size changes (ex. the URL bar being hidden) or the CSS viewport changes. I think the confusing part is that it's using old values. The list of cases that it deals with is actually pretty useful.

The naming is pretty crappy, ex. oldBrowserWidth is the old version of viewportW. Ideally it would be oldViewportW~viewportW but it's taken from browser.js. I'd rather fix naming issues and less important design issues in a followup, possibly in bug 793462.
Comment on attachment 663789 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h

>+    // Recalculates the display state, including the CSS viewport. This should
>+    // be called whenever we believe the meta viewport data on a document may
>+    // have changed. If it didn't change, this function doesn't do anything.
>+    // However, it should not be called all the time as it is fairly expensive.
>+    void HandlePossibleMetaViewportChange();
>+

This is still a misnomer because this state isn't always related to
<meta viewport>.  Just drop the "Meta" from the name here and the
mention of "meta viewport" in the comment.

r=me with that change.
Attachment #663789 - Flags: review?(jones.chris.g) → review+
Comment on attachment 663789 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

,
>                    uint32_t aAppId)
>-  : mRemoteFrame(nullptr)
>+  : mLastURI(nullptr)
No need to initialize nsCOMPtr to nullptr

>+NS_IMETHODIMP
>+TabChild::OnLocationChange(nsIWebProgress* aWebProgress,
>+                           nsIRequest* aRequest,
>+                           nsIURI *location,
nsIURI* aLocation


>+                           uint32_t aFlags)
>+{
>+  if (!IsAsyncPanZoomEnabled()) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMWindow> window;
>+  aWebProgress->GetDOMWindow(getter_AddRefs(window));
>+  if (!window) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMDocument> progressDoc;
>+  window->GetDocument(getter_AddRefs(progressDoc));
>+  if (!progressDoc) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  mWebNav->GetDocument(getter_AddRefs(domDoc));
>+  if (!domDoc || !SameCOMIdentity(domDoc, progressDoc)) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIURIFixup> urifixup(do_GetService(NS_URIFIXUP_CONTRACTID));
>+  if (!urifixup) {
>+    return NS_OK;
>+  }
I don't understand the need for urifixup.


>+
>+  Element* htmlElement = document->GetHtmlElement();
>+  Element* bodyElement = document->GetBodyElement();
>+
>+  nsCOMPtr<nsIDOMElement> htmlDOMElement;
>+  nsCOMPtr<nsIDOMElement> bodyDOMElement;
>+
>+  if (htmlElement) {
>+    CallQueryInterface(htmlElement, getter_AddRefs(htmlDOMElement));
>+  }
>+  if (bodyElement) {
>+    CallQueryInterface(bodyElement, getter_AddRefs(bodyDOMElement));
>+  }
Why not
nsCOMPtr<nsIDOMElement> htmlDOMElement = do_QueryInterface(document->GetHtmlElement());
nsCOMPtr<nsIDOMElement> bodyDOMElement = do_QueryInterface(document->GetBodyElement());

>+    if (domDoc) {
>+      domDoc->GetDocumentElement(getter_AddRefs(docElement));
>+      if (docElement) {
>+        utils->SetDisplayPortForElement(
>+         aFrameMetrics.mDisplayPort.x, aFrameMetrics.mDisplayPort.y,
>+         aFrameMetrics.mDisplayPort.width, aFrameMetrics.mDisplayPort.height,
>+         docElement);

2 space indentation, please

>@@ -1389,16 +1715,19 @@ TabChild::InitRenderingState()
> 
>     if (observerService) {
>         observerService->AddObserver(this,
>                                      "cancel-default-pan-zoom",
>                                      false);
>         observerService->AddObserver(this,
>                                      "browser-zoom-to-rect",
>                                      false);
>+        observerService->AddObserver(this,
>+                                     "before-first-paint",
>+                                     false);
>     }
Argh, observer service will keep the TabChild alive until shutdown.
I know you're just copying the existing code, but we really must fix the issue.
The original code seems to be coming from Bug 775447 and Bug 775448, which you might
know something about ;)
Attachment #663789 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #53)
> Comment on attachment 663789 [details] [diff] [review]
> Argh, observer service will keep the TabChild alive until shutdown.
> I know you're just copying the existing code, but we really must fix the
> issue.
> The original code seems to be coming from Bug 775447 and Bug 775448, which
> you might
> know something about ;)

Filed follow-up bug 794733. This is definitely a problem, but it's outside the scope of this bug.

/actually does his followups
(In reply to Olli Pettay [:smaug] from comment #53)
> Comment on attachment 663789 [details] [diff] [review]
> Add support for <meta name=viewport> on B2G/async panning and zooming
> I don't understand the need for urifixup.

See bug 747883. This code exists in Fennec's browser.js. I'm wary of getting rid of it due to how much work has gone into it in terms of bug fixes and such.
Dealt with code review, not sure how to carry r+ from cjones.
Attachment #663789 - Attachment is obsolete: true
Attachment #665645 - Flags: review?(jones.chris.g)
Attachment #665645 - Flags: review?(bugs)
Comment on attachment 665645 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

Carrying r+ from cjones.
Attachment #665645 - Flags: review?(jones.chris.g) → review+
Added RemoveObserver calls for everything using the observer service in TabChild.
Attachment #665645 - Attachment is obsolete: true
Attachment #665645 - Flags: review?(bugs)
Attachment #665674 - Flags: review?(jones.chris.g)
Attachment #665674 - Flags: review?(bugs)
Comment on attachment 665674 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

Carrying r+ from cjones.
Attachment #665674 - Flags: review?(jones.chris.g) → review+
Comment on attachment 665674 [details] [diff] [review]
Add support for <meta name=viewport> on B2G/async panning and zooming

>+    nsIDOMWindowUtils* GetDOMWindowUtils()
already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils()


>+    {
>+        nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(mWebNav);
>+        nsCOMPtr<nsIDOMWindowUtils> utils = do_GetInterface(window);
>+        return utils;
  return utils.forget();
And I'd prefer 2 space indentation.
TabChild,* are using random spacing bug Gecko coding style says 2 spaces.
Attachment #665674 - Flags: review?(bugs) → review+
There's a reftest failure on bug 784908, which blocks this, that I'm still dealing with. I don't have a lot of time to fix this and I don't know what the problem is, so landing this by Friday night is looking kind of risky.
I'm one of the few people on earth who are able to run android tests locally, so I'll try to help out later tonight.
https://hg.mozilla.org/mozilla-central/rev/1bf3d3d3cbe0
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
This sounds like something particularly important that were going to want to make sure we get right. Do we know of at least 10 top sites that make use of meta name viewport? Our QA probably should take a look at these sites to make sure meta name viewport is working with some top sites.
I agree. I don't know of any top sites that use this though. cc'ing Brad from the mobile team who might know. Or at least know someone who knows.
Keywords: verifyme
Doug might have good ideas too.
QA Contact: jsmith
Basically any site with a "mobile" version uses <meta name="viewport">.  This includes Google, Yahoo, Facebook, Amazon, eBay, Wikipedia, Twitter, Bing...
And we can detect if our <meta viewport> support is working by checking if the site renders at the right size, vs. "zoomed out"?
Naoki is hammering away at this. I'll keep a look out as well during my web compat testing.
QA Contact: jsmith → nobody
(In reply to Jonas Sicking (:sicking) from comment #69)
> And we can detect if our <meta viewport> support is working by checking if
> the site renders at the right size, vs. "zoomed out"?

Correct.  It depends on the exact contents of the tag, but in the usual case the layout should adapt to the device width, and the page should start at a normal (100%) zoom level rather than zoomed out.
See bug 795657 before proceeding on testing.
Test cases have been created for this: see moztrap.mozilla.org suite: browser product: b2g, tags : gaia, b2g, browser, topsites, viewport

Noticed odd painting on something that didn't have the viewport set : 
http://people.mozilla.com/~nhirata/html_tp/ which may have something to do with bug 795657?
Otoro phone, build 2012-10-23
Taken from default.xml in b2g-distro: 
* "platform_build" revision= db539a3bd139c93c09b0cd1c3f9396b74d68717c
* "gaia" revision= 0b8ec9b8c16429dc35453dbb7b9342fab3dd18fb 
* "releases-mozilla-aurora" revision= f58edfde05cb708f8a2c440d338f2e429aaf372b
* "gonk-misc" revision= db0c751715f4696515735eb1e0dc5df7a40eb81d
Status: RESOLVED → VERIFIED
Blocks: 807457
Blocks: 830306
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: