Much of the stuff in nsContentUtils.cpp relating to calculating the viewport would probably be better suited in a separate class. In addition, some of the work I'm doing in bug 756518 would be more cohesive if this were the case. This is a refactoring to make nsContentUtils::ViewportInfo a separate class, nsViewportInfo.

Refactoring of the viewport structure into a class.

Comment on attachment 692531 [details] [diff] [review] b821801 Review of attachment 692531 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but a few comments below I'd like to see addressed first. ::: content/base/public/nsViewportInfo.h @@ +23,5 @@ > + * nsContentUtils::GetViewportInfo for more information on this functionality. > + */ > +class NS_STACK_CLASS nsViewportInfo > +{ > + friend class nsContentUtils; Is this friendship needed? I didn't see anything in nsContentUtils that required it. ::: content/base/src/nsContentUtils.cpp @@ +5052,1 @@ > return ret; This constructor sets nsViewportSize.mAutoSize to false but the init code you removed here set it to true previously. @@ +5056,5 @@ > > nsAutoString handheldFriendly; > aDocument->GetHeaderData(nsGkAtoms::handheldFriendly, handheldFriendly); > if (handheldFriendly.EqualsLiteral("true")) { > + nsViewportInfo ret(aDisplayWidth, aDisplayHeight); Same here.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2) > Looks good, but a few comments below I'd like to see addressed first. Cool, thanks for the review. I addressed these comments and will post a new patch shortly. > Is this friendship needed? I didn't see anything in nsContentUtils that > required it. No, probably not any longer. I removed it. I originally added it because I was going to support accessing the member variables directly from nsContentUtils, but then I decided this wasn't necessary, but forgot to remove the friend. > This constructor sets nsViewportSize.mAutoSize to false but the init code > you removed here set it to true previously. Ah. Good catch. I changed this so the constructor inits it to "true" instead.

Comment on attachment 693004 [details] [diff] [review] b821801 (v2) Review of attachment 693004 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e75c8b88e65e

The patch for this bug needs to go into b2g18 if we want to uplift the patches for bug 862763.

Comment on attachment 693004 [details] [diff] [review] b821801 (v2) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): none, but this patch is required before the patch for bug 862763 can land. User impact if declined: none - this is a very low risk refactoring Testing completed: on release channel as of mozilla 20 Risk to taking this patch (and alternatives if risky): almost none String or UUID changes made by this patch: none

Triage 5/20 - Leo+ per comment 8