1145858 - Rename FirstrunPane to be less confusing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

The hierarchy of firstrun is confusing: FirstrunPane, FirstrunPager, FirstrunPanel

FirstrunPane: this is a container for the pager and the whole first run experience and is for animation purposes. The name of this class should reflect that. Perhaps FirstrunContainer, or AnimatedContainer, or something along those lines.

FirstrunPager is the ViewPager that contains the first run pages, but for consistency with HomePager/HomePanel, these pages are called FirstrunPanel.

There should also be some class docs clarifying the relationship between these.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Just your nagging conscience here. This refactor work should be secondary to any other bugs that need higher priority. Also remember landing this before other patches makes uplift the other patches harder.

Comment 2

[Chenxia Liu [:liuche]]

Aaaahh I also realize that there is a firstrun/FirstrunPanel as well as a static inner class FirstrunPanel! The static inner class should be renamed something like PanelConfig.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: renameFirstRunPaneReviewCopy

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8604326 [details] [diff] [review]
renameFirstRunPaneReviewCopy

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

Needs a little more work.

For refactoring, you should always use 'hg rename' to rename files (or hg copy, if it's relevant), which will preserve file history for hg annotate.

Per comment #0, can you also add a class comment describing the relationship between these files? Probably to FirstrunAnimationContainer.

Also need to address comment #2.

Comment 5

[Chenxia Liu [:liuche]]

This isn't a user-facing issue, but I do think it makes working with this part of the code base more confusing. We could keep it open and I'll try to find time to work on it, or we could WONTFIX it and I'll reopen if it bothers me too much. (Or I'll WONTFIX it if I don't get to it in the next two weeks.)

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Bug 1145858 - Rename FirstrunPane to be less confusing

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/1-2/

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/2-3/

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

https://reviewboard.mozilla.org/r/26889/#review24379

Looks good, thank you for the class comments! One last thing - be sure to propagate the rename changes to moz.build too, otherwise you'll get build errors.

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/2-3/

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

https://reviewboard.mozilla.org/r/26889/#review24489

Great! Do you have permissions to land this on fx-team? If not, flag 'checkin-needed' in the the keywords, or I can land this for you.

Comment 12

[Chenxia Liu [:liuche]]

Also, make sure to put the reviewer r=liuche in the commit message (or look at https://treeherder.mozilla.org/#/jobs?repo=fx-team to see what the commit messages are like).

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/3-4/

Comment 14

[Andrzej Hunt :ahunt]

(checkin not needed yet - doing a try build first)

Comment 15

[Andrzej Hunt :ahunt]

Successful try completed (my push-to-try seems to have been borked and hence never notified bugzilla):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=851316fba3d6&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=coalesced&filter-resultStatus=pending&filter-resultStatus=running

Comment 16

[Carsten Book [:Tomcat]]

Hi, this caused problems during checkin,

patching file mobile/android/base/firstrun/FirstrunAnimationContainer.java
Hunk #1 FAILED at 13
Hunk #2 FAILED at 64
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunAnimationContainer.java.rej
unable to find 'mobile/android/base/firstrun/FirstrunPager.java' for patching
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunPager.java.rej
unable to find 'mobile/android/base/firstrun/FirstrunPanel.java' for patching
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunPanel.java.rej
mobile/android/base/firstrun/FirstrunPane.java not tracked!
patch failed to apply
abort: fix up the merge and run hg transplant --continue

can you take a look, thanks!

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/4-5/

Comment 18

[Andrzej Hunt :ahunt]

(In reply to Carsten Book [:Tomcat] from comment #16)
> Hi, this caused problems during checkin,
> 
<snip>
> can you take a look, thanks!

Fixed now - we had some path changes recently which is what caused the patch to not apply!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/96c407f2815f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96c407f2815f