Closed Bug 1489910 Opened 6 years ago Closed 6 years ago

Rename remaining uses of "SPCSPS" (short for "scroll-position clamping scroll port size") to "visual viewport size"

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: JanH, Assigned: arika.arnzen, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: good-first-bug, Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1471708 +++

The MobileViewportManager still has a few instances of the abbreviated form (SPCSPS), which probably ought to be renamed as well (to VVPS).
Thanks for pointing that out. This should make a good first bug.
Mentor: botond
Keywords: good-first-bug
Summary: Rename the "scroll-position clamping scroll port size" to the "visual viewport size" → Rename remaining uses of "SPCSPS" (short for "scroll-position clamping scroll port size") to "visual viewport size"
Whiteboard: [lang=c++] → [gfx-noted] [lang=c++]
Is anyone working on this? I would like to!
(In reply to arika.arnzen from comment #2)
> Is anyone working on this? I would like to!

Great, thanks for your interest!

The first step is to build Firefox locally. Instructions can be found here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Let me know when you've done that and I'll post some more details about how to proceed.
I have already done this:)
Ok, great! I've assigned the bug to you.

The idea in this bug is that we've recently (in bug 147170) renamed a quantity called the "scroll-position clamping scroll port size" to "visual viewport size", but there are still some places in the code that use the abbreviated form "SPCSPS".

Here's what I suggest doing:

  - Use our Searchfox code search tool [1] to find uses of
    "SPCSPS" in the codebase.

  - Change the "SPCSPS" to "VisualViewportSize". (I think that's
    short enough that we don't need to use an opaque acronym 
    like "VVS"). Occurrences of "SPCSPS" in comments can be
    changed to "visual viewport size".

  - Rebuild after making the changes. Note that the rebuild will
    be much shorter than the initial build was, because only
    modified files will be rebuilt.

  - Commit the resulting changes and submit the patch for review.
    Instructions for that can be found here [2].

Let me know if you have any questions!

[1] http://searchfox.org/
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed
Assignee: nobody → arika.arnzen
Priority: -- → P3
Is it okay if I just attach the patch here, or do I need to commit and push my code? If you would like me to try committing, can you please comment on this bug as a voucher so I can get commit access? https://bugzilla.mozilla.org/show_bug.cgi?id=1489319
(In reply to arika.arnzen from comment #6)
> Is it okay if I just attach the patch here, or do I need to commit and push
> my code?

You can either attach the patch here, or use our Phabricator code review system (documentation for Phabricator can be found here [1]).

Neither of these require any level of commit access.

> If you would like me to try committing, can you please comment on
> this bug as a voucher so I can get commit access?
> https://bugzilla.mozilla.org/show_bug.cgi?id=1489319

I appreciate your eagerness to get Level 1 commit access, but this is not required for contributing changes, and it's usually a step taken after making several contributions, so I'm going to hold off on this for now. I'm happy to revisit this in the near future.

[1] https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
I will attach the patch here, thanks for clarifying about the commit access.
Attached patch tip.patch (obsolete) — Splinter Review
Comment on attachment 9008617 [details] [diff] [review]
tip.patch

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

Thanks, this generally looks good!

A few minor comments:

::: layout/base/MobileViewportManager.cpp
@@ +289,1 @@
>                                      const CSSToScreenScale& aZoom)

Please update the indentation here to keep the parameters aligned.

@@ +336,3 @@
>  {
>    // This function is a subset of RefreshViewportSize, and only updates the
> +  // VisualViewportSize.

As this is a comment, we can say "visual viewport size".

::: layout/base/MobileViewportManager.h
@@ +52,5 @@
>     * presShell.*/
>    void RequestReflow();
>  
>    /* Notify the MobileViewportManager that the resolution on the presShell was
> +   * updated, and the VisualViewportSize needs to be updated. */

Likewise, "visual viewport size".

@@ +66,5 @@
>    /* Main helper method to update the CSS viewport and any other properties that
>     * need updating. */
>    void RefreshViewportSize(bool aForceAdjustResolution);
>  
> +  /* Secondary main helper method to update just the VisualViewportSize. */

Likewise.

@@ +86,5 @@
>                                               const mozilla::ScreenIntSize& aDisplaySize,
>                                               const mozilla::CSSSize& aViewport,
>                                               const mozilla::Maybe<float>& aDisplayWidthChangeRatio);
>  
>    /* Updates the scroll-position-clamping scrollport size */

We should change this comment to say "updates the visual viewport size". However, since that's literally the name of the function, the comment is redundant, so let's just remove it.

@@ +92,1 @@
>                      const mozilla::CSSToScreenScale& aZoom);

Please update the indentation here to keep the parameters aligned.
Attachment #9008617 - Flags: feedback+
Attached patch tip2.patchSplinter Review
Here are the most recent changes!
Thanks!

By the way, a couple of useful Bugzilla features for future reference:

* When uploading a new patch, you can make the old patch as "obsolete" in the attachment details
* There is a "review" flag you can set on a patch to indicate that it's ready for review, and select a reviewer
Attachment #9008617 - Flags: feedback+
Attachment #9008617 - Attachment is obsolete: true
Comment on attachment 9009229 [details] [diff] [review]
tip2.patch

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

Looks good, thanks!
Attachment #9009229 - Flags: review+
I pushed the patch to our Try server, to make sure it's building and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c196c7cbe66d170150ecdd89b330af67cee64bbd
The Try push is looking good, so I went ahead and committed the patch to mozilla-inbound.

It should merge over to mozilla-central within a day or so.
Awesome. Thanks for all your help!
I accidentally committed the patch with the wrong bug number in the commit message (1488910 instead of 1489910). Sorry about that.

Here is the mozilla-inbound commit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f67a586878d
And it has now merged to mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/1f67a586878d

Thanks for your contribution here, Arika!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: