Closed Bug 695332 Opened 8 years ago Closed 8 years ago

Factor out browser toolbar into a self-contained view

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: lucasr, Unassigned)

Details

Attachments

(2 files)

No description provided.
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+
Pushed with the nits fixed: http://hg.mozilla.org/projects/birch/rev/0a783dbfb864
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
(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.
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.
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 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+
(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 :-)
You need to log in before you can comment on or make changes to this bug.