Closed Bug 821801 Opened 7 years ago Closed 7 years ago

Refactor ViewportInfo and separate into a class

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g leo+
Tracking Status
b2g18 ? fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: jwir3, Assigned: jwir3)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch b821801 (obsolete) — Splinter Review
Refactoring of the viewport structure into a class.
Attachment #692531 - Flags: review?(bugmail.mozilla)
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.
Attachment #692531 - Flags: review?(bugmail.mozilla) → review-
Attached patch b821801 (v2)Splinter Review
(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.
Attachment #692531 - Attachment is obsolete: true
Attachment #693004 - Flags: review?(bugmail.mozilla)
Comment on attachment 693004 [details] [diff] [review]
b821801 (v2)

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

Looks good, thanks.
Attachment #693004 - Flags: review?(bugmail.mozilla) → review+
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/e75c8b88e65e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The patch for this bug needs to go into b2g18 if we want to uplift the patches for bug 862763.
Blocks: 862763
tracking-b2g18: --- → ?
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
Attachment #693004 - Flags: approval-mozilla-b2g18?
blocking-b2g: --- → leo?
Triage 5/20 - Leo+ per comment 8
blocking-b2g: leo? → leo+
Attachment #693004 - Flags: approval-mozilla-b2g18?
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.