Factor out browser toolbar into a self-contained view

RESOLVED FIXED

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: lucasr, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Comment 1

7 years ago
Created attachment 567753 [details] [diff] [review]
Factor out browser toolbar into a self-contained view
Attachment #567753 - Flags: review?(mark.finkle)
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

7 years ago
Pushed with the nits fixed: http://hg.mozilla.org/projects/birch/rev/0a783dbfb864
Status: NEW → RESOLVED
Last Resolved: 7 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.
(Reporter)

Comment 5

7 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.
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

7 years ago
Created attachment 567950 [details] [diff] [review]
Fix browser toolbar id

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+
(Reporter)

Comment 9

7 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 :-)
You need to log in before you can comment on or make changes to this bug.