Closed
Bug 1040444
Opened 10 years ago
Closed 10 years ago
[Lint] datetime_picker lint and performance warnings
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: rnewman, Assigned: rahul.parsani, Mentored)
Details
(Whiteboard: lint, [good first bug])
Attachments
(1 file, 2 obsolete files)
13.45 KB,
patch
|
rahul.parsani
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8461135 -
Flags: review?(wjohnston)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
Forgot to include a commit message.
Attachment #8463047 -
Attachment is obsolete: true
Attachment #8468042 -
Flags: review+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Assignee: nobody → rahul.parsani
Comment 6•10 years ago
|
||
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]
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d77efd20ae9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: lint, [good first bug][fixed-in-fx-team] → lint, [good first bug]
Target Milestone: --- → Firefox 34
Comment 8•10 years ago
|
||
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).
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•