Closed
Bug 695332
Opened 11 years ago
Closed 11 years ago
Factor out browser toolbar into a self-contained view
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: lucasr, Unassigned)
Details
Attachments
(2 files)
20.14 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
sriram
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #567753 -
Flags: review?(mark.finkle)
Comment 2•11 years ago
|
||
Comment on attachment 567753 [details] [diff] [review] Factor out browser toolbar into a self-contained view Brian has a patch to improve the camelCase and XML id's. Please make any overlapping updates from his patch to yours. r+ with those changes.
Attachment #567753 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 3•11 years ago
|
||
Pushed with the nits fixed: http://hg.mozilla.org/projects/birch/rev/0a783dbfb864
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 4•11 years ago
|
||
There are few problems with this code. All the id's are using "@android:+id" -> which creates id's in android namespace and not in our application's namespace. This is not a good practice as it messes up with the android's default's ids. Like the favicon had issues for Brian, as the id was in android's namespace. The android namespace can be used for things where we use the android's native stuff (like the "list" in ListActivity). Also, the style BrowserToolbar is pretty common with many other layout in other xmls. It's better to have a common VerticalLayout and HorizontalLayout for the sake of resuability.
Reporter | ||
Comment 5•11 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #4) > There are few problems with this code. All the id's are using "@android:+id" > -> which creates id's in android namespace and not in our application's > namespace. This is not a good practice as it messes up with the android's > default's ids. Like the favicon had issues for Brian, as the id was in > android's namespace. The android namespace can be used for things where we > use the android's native stuff (like the "list" in ListActivity). I fixed the ids before pushing. See the link to the actual commit. > Also, the style BrowserToolbar is pretty common with many other layout in > other xmls. It's better to have a common VerticalLayout and HorizontalLayout > for the sake of resuability. I'm pretty sure we'll end up adding BrowserToolbar-specific style bits once we implement the actual design for it. So, no need to generalize it for now.
Comment 6•11 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/0a783dbfb864#l5.31 - This needs to be fixed. And, it's better to use underscores with ids, styles, themes, and other resources. Camel case is better with attr names and style names as they are compiled into classes.
Reporter | ||
Comment 7•11 years ago
|
||
Oh, true. Let this one slip out somehow. Thanks for pointing it out. Here's a patch to fix it.
Attachment #567950 -
Flags: review?(sriram)
Comment 8•11 years ago
|
||
Comment on attachment 567950 [details] [diff] [review] Fix browser toolbar id Is this valid still? I somehow forgot to review this. Sorry :(
Attachment #567950 -
Flags: review?(sriram) → review+
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #8) > Comment on attachment 567950 [details] [diff] [review] > Fix browser toolbar id > > Is this valid still? I somehow forgot to review this. Sorry :( This is ooooold. Ignore it :-)
Assignee | ||
Updated•1 year 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
•