Closed Bug 1026010 Opened 8 years ago Closed 8 years ago

Rename GeckoApp.ACTION_BOOKMARK to something more intuitive

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 33

People

(Reporter: lucasr, Assigned: shashank, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

BUG1026010.patch [Updated] - Rename GeckoApp.ACTION_BOOKMARK to ACTION_HOMESCREEN_SHORTCUT
7.87 KB, patch
Details | Diff | Splinter Review 
GeckoApp.ACTION_HOMESCREEN or something. We might have to keep the actual value of the String constant to avoid breaking existing shortcuts.
Whiteboard: [mentor=lucasr][lang=java]
(In reply to Lucas Rocha (:lucasr) from comment #0)
> GeckoApp.ACTION_HOMESCREEN or something. We might have to keep the actual
> value of the String constant to avoid breaking existing shortcuts.

1) The declaration 'public static final String ACTION_BOOKMARK = "org.mozilla.gecko.BOOKMARK";' seems alright. Am I missing something?

2) The variable is referenced across 3 files in 5 files (MXR). I beleive these all should be updated too. Am I right?

3) What is the most suitable name?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Shashank VRSN Sabniveesu from comment #1)
> (In reply to Lucas Rocha (:lucasr) from comment #0)
> > GeckoApp.ACTION_HOMESCREEN or something. We might have to keep the actual
> > value of the String constant to avoid breaking existing shortcuts.
> 
> 1) The declaration 'public static final String ACTION_BOOKMARK =
> "org.mozilla.gecko.BOOKMARK";' seems alright. Am I missing something?

This is bug is about renaming the ACTION_BOOKMARK constant with a more meaningful name (and leave the value itself unchanged to avoid breaking existing shortcuts).

> 2) The variable is referenced across 3 files in 5 files (MXR). I beleive
> these all should be updated too. Am I right?

Yep.

> 3) What is the most suitable name?

Let's go with ACTION_HOMESCREEN_SHORTCUT.
Flags: needinfo?(lucasr.at.mozilla)
Mentor: lucasr.at.mozilla
Whiteboard: [mentor=lucasr][lang=java] → [lang=java]
Assignee: nobody → shashank
Mentor: lucasr.at.mozilla
Attachment #8441606 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(lucasr.at.mozilla)
Mentor: lucasr.at.mozilla
Comment on attachment 8441606 [details] [diff] [review]
Bug 1026010 - Rename GeckoApp.ACTION_BOOKMARK to ACTION_HOMESCREEN_SHORTCUT

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

Looks good. You'll have to rebase this patch with latest m-c as I've changed GeckoApp a bit.
Attachment #8441606 - Flags: review?(lucasr.at.mozilla) → review+
No need to needinfo *and* request patch review btw :-) Pick one or the other depending on the type of request.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #5)
> No need to needinfo *and* request patch review btw :-) Pick one or the other
> depending on the type of request.

:) A few bugs ago, the mentor said that 'needinfos' alone catch his/her sight fast. Fine, I'll do away with the practice
(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8441606 [details] [diff] [review]
> Bug 1026010 - Rename GeckoApp.ACTION_BOOKMARK to ACTION_HOMESCREEN_SHORTCUT
> 
> Review of attachment 8441606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. You'll have to rebase this patch with latest m-c as I've changed
> GeckoApp a bit.

i still could apply the patch after m-c pull at the point of this writing. Should I wait for your changes to appear in a later pull?
Flags: needinfo?(lucasr.at.mozilla)
I think the changes I mentioned have already landed in m-c btw.
Flags: needinfo?(lucasr.at.mozilla)
Want me to submit a TRY run?
What else are we waiting for?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Shashank VRSN Sabniveesu from comment #9)
> Want me to submit a TRY run?
> What else are we waiting for?

Yep, please submit a try run.
Flags: needinfo?(lucasr.at.mozilla)
Updated as one change is missing from previously r+ed patch; inferred from TRY run.

New TRY run: https://tbpl.mozilla.org/?tree=Try&rev=871bf20a36a5 -- All tests passed
Attachment #8441606 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/45637492da9d
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
Why is the status still 'NEW"?

(asking to improve my awareness)
Flags: needinfo?(ryanvm)
Bugs aren't resolved until they're merged to mozilla-central. fx-team is one of a few different staging branches where we verify that everything builds and passes tests before going into production. If it makes you feel better, I changed it to ASSIGNED now at least :P
Status: NEW → ASSIGNED
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> If it makes you feel better,
> I changed it to ASSIGNED now at least :P

Haha.. Thanks for that! :)
https://hg.mozilla.org/mozilla-central/rev/45637492da9d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 33
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.