1060294 - [bluetooth] Update new settings.html with gaia-header

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

[Wilson Page [:wilsonpage]]

Since our gaia-header patch landed, a new settings.html document was made using the old <header> building block. This needs to be converted to use gaia-header

Comment 1

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 2

[Stephany Wilkes]

Thank you, Wilson. If we can get review on this on Monday (a holiday in U.S. and Canada) it should still be able to make FL. So close!

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://github.com/mozilla-b2g/gaia/commit/3c04e67480a2f64b99d7a5cf84071f801f3d9a40

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

Reverted for breaking settings navigation and the settings integration test bluetooth_settings_test.js.

Caused failures like this:
https://tbpl.mozilla.org/php/getParsedLog.php?id=47112673&tree=B2g-Inbound#error1


master: https://github.com/mozilla-b2g/gaia/commit/2be78d83a760fa3b9638fe51c266b442d14597f1

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

make sure to update the query selector here when relanding: https://github.com/mozilla-b2g/gaia/blob/91d0294120eaa35a60f84fe82c7d89d2e01902cb/apps/settings/test/marionette/app/bluetooth_app.js#L17

Comment 6

[Yan Or]

Attachment: Updated PR

Comment 7

[Yan Or]

Updated Wilson's PR to use gaia-subheader and fixed integration tests.

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Who did the initial review for this? If I can verify the bluetooth parts got reviewed earlier, I don't mind r+ the test fixes. But I don't see it in this bug :/

Comment 9

[Yan Or]

I'm not sure.  Wilson handled the initial patch and he's on PTO until the 8th.  I see mikeh landed this on the 30th but I don't know who gave him the go ahead.

Stephany, do you know the details or can you track down someone to review this?  iliu hasn't responded on this so far.

Comment 10

[Stephany Wilkes]

Comment on attachment 8481897 [details] [review]
Updated PR

Per chat with Wilson before his PTO, assigned to gmarty. Please take this ASAP... it's merge day.

Comment 11

[Stephany Wilkes]

And... Guillaume is in London. Michael, you may be our only hope. These time zones are absolutely killing us on web component reviews in particular. :(

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8481897 [details] [review]
Updated PR

I can't review this since I have no idea about the bluetooth code :( I'm still unclear why this landed without a bluetooth peer review (or any review at all?). Review might've caught the settings breakage.

Also, we have already branched 2.1, so whenever this does get r+ it's going to need uplift approval as well.

Comment 13

[[e-mail], [e-mail]]

Comment on attachment 8481897 [details] [review]
Updated PR

I'm sorry about to review the patch late because the new architecture of embedded Bluetooth app is broken in these day. Since bug 900551 fixed permission checking, we can not use embedded iframe in settings app in v2.1. It will kill embedded Bluetooth app. Then, kill settings app too. So that bug 1061437 already reverted embedded Bluetooth app in settings app. Cause so much inconvenience, not what we would like to see.

In other word, the patch of this bug will not be executed. It will run into settings/elements/bluetooth.html as before. But we still want to re-enabled embedded Bluetooth in v2.2 while nested OOP is ready to support. Then, Yor's patch will be ready in the future while embedded Bluetooth app is re-enabled.

Yor, could you please update the patch for the latest code base? Thanks.

Comment 14

[Yan Or]

Ok. Since we've already branched, this can wait til 2.2.

Comment 15

[Guillaume Marty [:gmarty]]

Comment on attachment 8481897 [details] [review]
Updated PR

Clearing the review flag for now.

Comment 16

[Wilson Page [:wilsonpage]]

mhenretty: I wrote the initial patch for this and I think due to a communication mixup :mikeh assumed it was ready to land. Apologies, this was not my intention, I was expecting it to go through review first.

Comment 17

[BMO Automation]

Firefox OS is not being worked on