695332 - Factor out browser toolbar into a self-contained view

Status: RESOLVED FIXED

Description

[Lucas Rocha (:lucasr)]

Comment 1

[Lucas Rocha (:lucasr)]

Created attachment 567753 [details] [diff] [review]
Factor out browser toolbar into a self-contained view

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Lucas Rocha (:lucasr)]

Pushed with the nits fixed: http://hg.mozilla.org/projects/birch/rev/0a783dbfb864

Comment 4

[Sriram Ramasubramanian [:sriram]]

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.

Comment 5

[Lucas Rocha (:lucasr)]

(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

[Sriram Ramasubramanian [:sriram]]

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.

Comment 7

[Lucas Rocha (:lucasr)]

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.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 567950 [details] [diff] [review]
Fix browser toolbar id

Is this valid still? I somehow forgot to review this. Sorry :(

Comment 9

[Lucas Rocha (:lucasr)]

(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 :-)