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)
Core
Panning and Zooming
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)
7.73 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
+++ 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).
Comment 1•6 years ago
|
||
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++]
Assignee | ||
Comment 2•6 years ago
|
||
Is anyone working on this? I would like to!
Comment 3•6 years ago
|
||
(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.
Assignee | ||
Comment 4•6 years ago
|
||
I have already done this:)
Comment 5•6 years ago
|
||
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
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•6 years ago
|
||
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
Comment 7•6 years ago
|
||
(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
Assignee | ||
Comment 8•6 years ago
|
||
I will attach the patch here, thanks for clarifying about the commit access.
Assignee | ||
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
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+
Assignee | ||
Comment 11•6 years ago
|
||
Here are the most recent changes!
Comment 12•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #9008617 -
Flags: feedback+
Updated•6 years ago
|
Attachment #9008617 -
Attachment is obsolete: true
Comment 13•6 years ago
|
||
Comment on attachment 9009229 [details] [diff] [review] tip2.patch Review of attachment 9009229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Attachment #9009229 -
Flags: review+
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
Awesome. Thanks for all your help!
Comment 17•6 years ago
|
||
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
Comment 18•6 years ago
|
||
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
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•