Closed Bug 1261041 Opened 5 years ago Closed 4 years ago

Remove feature11Plus - feature15Plus flags

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: sebastian, Assigned: shatur, Mentored)

References

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(1 file, 1 obsolete file)

Mentor: s.kaspari
Whiteboard: [good next bug][lang=java]
Attached patch Raw.patch (obsolete) — Splinter Review
Hi Sebastian

  I have some queries related to this bug. Like, removing constant flag feature11Plus, do I have to remove all related code or do I have to change its occurrences[everywhere] to feature15Plus[as feature11Plus also includes feature15]?
  I have uploaded a raw patch. Please let me know, if I am on right track.

Thanks!!
Flags: needinfo?(s.kaspari)
Hey shatur! :)

Our minSdkVersion is set to 15. So all checks for feature11Plus, ... feature15Plus will return true always. So you can remove the checks and the constants. No need to do any checks for this anymore.
Flags: needinfo?(s.kaspari)
Assignee: nobody → tushar.saini1285
Status: NEW → ASSIGNED
Comment on attachment 8793828 [details]
Bug 1261041 - Remove feature11Plus-feature15Plus flags.

https://reviewboard.mozilla.org/r/80448/#review80660

Thanks for the patch, Shatur. And sorry for delay reviewing it.

The patch is looking very good already. There are some things we need to clean up and then I think it's ready to land. :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3365
(Diff revision 1)
>              findInPage.setEnabled(false);
>  
>              // NOTE: Use MenuUtils.safeSetEnabled because some actions might
>              // be on the BrowserToolbar context menu.
> -            if (Versions.feature11Plus) {
> -                // There is no page menu prior to v11 resources.
> +            // There is no page menu prior to v11 resources.

We can remove this comment too now.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3447
(Diff revision 1)
> -        }
>          MenuUtils.safeSetEnabled(aMenu, R.id.subscribe, tab.hasFeeds());
>          MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, tab.hasOpenSearch());
>          MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, !isAboutHome(tab));
>  
>          // Action providers are available only ICS+.

This comment is now outdated too.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3583
(Diff revision 1)
>                  if (item.isChecked()) {
>                      Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.MENU, extra);
>                      tab.removeBookmark();
>                      item.setTitle(resolveBookmarkTitleID(false));
> -                    if (Versions.feature11Plus) {
> -                        // We don't use icons on GB builds so not resolving icons might conserve resources.
> +                    // We don't use icons on GB builds so not resolving icons might conserve resources.

Outdated comment

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3589
(Diff revision 1)
>                  } else {
>                      Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.MENU, extra);
>                      tab.addBookmark();
>                      item.setTitle(resolveBookmarkTitleID(true));
> -                    if (Versions.feature11Plus) {
> -                        // We don't use icons on GB builds so not resolving icons might conserve resources.
> +                    // We don't use icons on GB builds so not resolving icons might conserve resources.

Outdated comment

::: mobile/android/base/java/org/mozilla/gecko/DoorHangerPopup.java:326
(Diff revision 1)
>          // Make the popup focusable for accessibility. This gets done here
>          // so the node can be accessibility focused, but on pre-ICS devices this
>          // causes crashes, so it is done after the popup is shown.

Outdated comment

::: mobile/android/base/java/org/mozilla/gecko/db/AbstractTransactionalProvider.java:97
(Diff revision 1)
>      /**
>       * Return true if OS version and database parallelism support indicates
>       * that this provider should bundle writes into transactions.
>       */
>      @SuppressWarnings("static-method")
>      protected boolean shouldUseTransactions() {

This method is now obsolete (It only returns true after all). We could remove it. Maybe not in this bug but we could file a new one for it.

::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java:40
(Diff revision 1)
>  import java.util.List;
>  
>  public class TabQueueHelper {
>      private static final String LOGTAG = "Gecko" + TabQueueHelper.class.getSimpleName();
>  
>      // Disable Tab Queue for API level 10 (GB) - Bug 1206055

Outdated comment - This flag is now obsolete. We coulkd file a follow-up bug to remove it.

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:104
(Diff revision 1)
>      public class OnValueChangeListener implements NumberPicker.OnValueChangeListener {
>          @Override
>          public void onValueChange(NumberPicker picker, int oldVal, int newVal) {
>              updateInputState();
>              mTempDate.setTimeInMillis(mCurrentDate.getTimeInMillis());
> -            final boolean newBehavior = Versions.feature11Plus;
> +            final boolean newBehavior = true;

newBehavior is now unneeded and you can remove more of the code.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/Clipboard.java:66
(Diff revision 1)
>  
>      @WrapForJNI(calledFrom = "gecko")
>      public static void setText(final CharSequence text) {
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              @SuppressWarnings("deprecation")

This annotation can now be removed after we removed the deprecated code, I guess.
Attachment #8793828 - Flags: review?(s.kaspari)
Attachment #8793731 - Attachment is obsolete: true
Comment on attachment 8793828 [details]
Bug 1261041 - Remove feature11Plus-feature15Plus flags.

https://reviewboard.mozilla.org/r/80448/#review81060

This is looking good! Thanks for this patch. I'll trigger a try run from reviewboard.
Attachment #8793828 - Flags: review?(s.kaspari) → review+
There's only one unrelated failure on try. This can land.
Keywords: checkin-needed
Please close out the two open issues in MozReview so that this can autoland.
Keywords: checkin-needed
Closed the issues in mozreview.
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7914b9afee68
Remove feature11Plus-feature15Plus flags. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/7914b9afee68
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.