[Lint] datetime_picker lint and performance warnings

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rahul.parsani, Mentored)

Tracking

Trunk
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: lint, [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

Line 26:
Set android:baselineAligned="false" on this element for better performance

When a LinearLayout is used to distribute the space proportionally between nested layouts, the baseline alignment property should be turned off to make the layout computation faster.


Line 35:
Use a layout_width of 0dp instead of wrap_content for better performance

When only a single widget in a LinearLayout defines a weight, it is more efficient to assign a width/height of 0dp to it since it will absorb all the remaining space anyway. With a declared width/height of 0dp it does not have to measure its own size first.


Line 47:
Nested weights are bad for performance

Layout weights require a widget to be measured twice. When a LinearLayout with non-zero weights is nested inside another LinearLayout with non-zero weights, then the number of measurements increase exponentially.


Line 117:
[I18N] Hardcoded string ":", should use @string resource

Hardcoding text attributes directly in layout files is bad for several reasons:

* When creating configuration variations (for example for landscape or portrait)you have to repeat the actual text (and keep it up to date when making changes)

* The application cannot be translated to other languages by just adding new translations for existing string resources.

In Android Studio and Eclipse there are quickfixes to automatically extract this hardcoded string into a resource lookup.


Line 34:
This LinearLayout layout or its LinearLayout parent is possibly useless

A layout with children that has no siblings, is not a scrollview or a root layout, and does not have a background, can be removed and have its children moved directly into the parent for a flatter and more efficient layout hierarchy.
The string hardcoded string has been added to the strings.xml.in & android_strings.dtd files. 

For the other warnings, I moved the layout from 

datetime_picker (LinearLayout)
-spinners (LinearLayout)
--date_spinners (LinearLayout)
--time_spinners (LinearLayout)

to

datetime_picker (LinearLayout)
-date_spinners (LinearLayout)
-time_spinners (LinearLayout)

All behavior for the layout spinners was moved to the layout datetime_picker in DateTimePicker.java. Since datetime_picker became a vertical LinearLayout, there was no need to set android:baselineAligned="false" on this element.
Posted patch 1040444.patch (obsolete) — Splinter Review
Attachment #8461135 - Flags: review?(wjohnston)
Comment on attachment 8461135 [details] [diff] [review]
1040444.patch

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

This looks great. Thanks! A couple questions about the weights. If you remove them and everything is happy, upload a new patch for checkin!

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: mobile/android/base/resources/layout/datetime_picker.xml
@@ +40,1 @@
>          android:layout_weight="1"

Not your code, but I don't think we need weight here do we?

@@ +91,5 @@
> +        android:layout_height="wrap_content"
> +        android:orientation="horizontal"
> +        android:layout_marginLeft="1dip"
> +        android:layout_marginRight="1dip"
> +        android:layout_weight="1"

Same with the weight here.
Attachment #8461135 - Flags: review?(wjohnston) → review+
Posted patch 1040444.patch (obsolete) — Splinter Review
Removed the weights and everything looked fine in both portrait and landscape. Let me know if the patch is in the correct format. Thanks!
Attachment #8461135 - Attachment is obsolete: true
Posted patch 1040444.patchSplinter Review
Forgot to include a commit message.
Attachment #8463047 - Attachment is obsolete: true
Attachment #8468042 - Flags: review+
Assignee: nobody → rahul.parsani
https://hg.mozilla.org/integration/fx-team/rev/4d77efd20ae9

Thanks for the patch!
Keywords: checkin-needed
Whiteboard: lint, [good first bug] → lint, [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4d77efd20ae9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: lint, [good first bug][fixed-in-fx-team] → lint, [good first bug]
Target Milestone: --- → Firefox 34
A comment explaining where this string is used would help (and, unrelated to this patch, there's a typo in the immediate following line too).
You need to log in before you can comment on or make changes to this bug.