Closed
Bug 964653
(system-dialog)
Opened 11 years ago
Closed 11 years ago
[Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog)
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iliu, Assigned: iliu)
References
Details
(Whiteboard: in-bubble-tea)
Attachments
(1 file)
This is a refactor work for system dialog.
Assignee | ||
Updated•11 years ago
|
Alias: system-dialog
Assignee | ||
Updated•11 years ago
|
Summary: (system-dialog-window) [Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog) → [Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog)
Assignee | ||
Comment 1•11 years ago
|
||
This patch including of BluetoothPairingDialog. I will remove it to bug 859168. Please ignore this section.
Alive, could you please help to have an architecture review in this feedback patch? Thanks.
Attachment #8376096 -
Flags: feedback?(alive)
Comment 2•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
f+ for only SystemDialog part.
Attachment #8376096 -
Flags: feedback?(alive) → feedback+
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Alive, thanks for your guiding in the architecture. I have updated the patch. And add more comment in detail. Please help to review it, thanks. I will keep to add test later.
* Remove Bluetooth pairing dialog in this patch.
* Refactor system_simcard_dialog.
* Refactor fxa_dialog.
* Extends BaseUI for class `SystemDialog`.
* Add more comment for JSDoc.
* Revise loading sequence for these system dialogs.
Attachment #8376096 -
Flags: review?(alive)
Comment 4•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
https://travis-ci.org/mozilla-b2g/gaia/builds/19084074
Please fix the error before sending review.
Attachment #8376096 -
Flags: review?(alive)
Assignee | ||
Comment 5•11 years ago
|
||
Hi Julien,
May I ask you a question about jshint error "E:0200: Invalid JsDoc tag: requires"?(https://travis-ci.org/mozilla-b2g/gaia/jobs/19097447#L1087-L1088)
I'm able to get the error while doing `git commit -m ...` in my local side. In order to fix the error, I try to revise it via `* @requires module:AppWindowManager`
or remove all the comment above https://github.com/mozilla-b2g/gaia/pull/16282/files#diff-64f97262e745b967c87810a7dcb343edR27
or other syntax..
Then, I still got the error. Not sure what's the error for the `tag: requires` here. I have no idea beside of remove the error tag.
Flags: needinfo?(felash)
Comment 6•11 years ago
|
||
This is actually a gjslint error, not a jshint error.
Therefore, I have 2 possible solutions for you:
1- fix the jshint issues in this file, remove it from build/jshint/xfail.list, so that it's not checked by the buggy gjslint anymore
2- add "requires" to this line, in the "custom jsdoc" parameter : https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L869
My preference would be 1. of course, but 2. is probably quicker ;)
Flags: needinfo?(felash)
Assignee | ||
Comment 7•11 years ago
|
||
Julien,
Your first solution is working for me. Once removed it from build/jshint/xfail.list, the file would be enabled to check the hint. Then, I don't receive the gjslint error again. Thanks for your command. It's really useful.:)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Alive, I fixed the error. And add unit test for these additional files. Could you please help to review my patch? Thanks.
Attachment #8376096 -
Flags: review?(alive)
Comment 9•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Nearly done, please see github comments, thanks.
Attachment #8376096 -
Flags: review?(alive)
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Alive, I have updated my patch with your comment. Please help to review it again. Once you agree with my patch, I'll revise the unit test again. Thanks.
Attachment #8376096 -
Flags: review?(alive)
Comment 11•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Some more nits. Last round needed.
Attachment #8376096 -
Flags: review?(alive)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Revised some nits and leave comment on Github.
Alive, could you please review it again? Thank you so much.
Attachment #8376096 -
Flags: review?(alive)
Assignee | ||
Updated•11 years ago
|
Attachment #8376096 -
Attachment description: A feedback patch for pr 16282 → pull request 16282
Comment 13•11 years ago
|
||
Comment on attachment 8376096 [details] [review]
pull request 16282
Well done! Thanks for your patience to complete it!
r+ but you have two failing unit tests.
Attachment #8376096 -
Flags: review?(alive) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Thanks for your reviewing effort with so much patience. I will revise unit test base on the latest patch. And ensure travis build pass for landing process.
Assignee | ||
Comment 15•11 years ago
|
||
Updated unit test. Also add test case in layout manager since we implemented new method "availableHeight()". Looks like unit test passed, but failed in non-relative marionette test(notification tests system replace notification).
Assignee | ||
Comment 16•11 years ago
|
||
Travis build passed. Merged in Gaia/bubble-tea: https://github.com/mozilla-b2g/gaia/commit/c9a08d42366ce7ab8921db8c2f713e7616003c97
Assignee | ||
Updated•11 years ago
|
Whiteboard: in-bubble-tea
Assignee | ||
Comment 17•11 years ago
|
||
Since it break unit test on bootstrap.js revert from gaia/bubble-tea: 353dbebe205b262211a7d832a3f4e6f546b9c0e7
Assignee | ||
Comment 18•11 years ago
|
||
Added and fixed bootstrap unit test. Merged in Gaia/bubble-tea: b77957794099e4af3f51f37bf6b73463cbf36413
Comment 19•11 years ago
|
||
Ian, though we plan to merge bubble-tea to master in order,
due to many conflict occurred in this patch, please help create a PR, check travis is green and land it to master. Thanks.
Flags: needinfo?(iliu)
Assignee | ||
Comment 20•11 years ago
|
||
Fred, thanks for your remind and effort. Looks like sim-pin dialog is revised before. I have solved the conflict. And wait all the unit tests passed.:)
Flags: needinfo?(iliu)
Assignee | ||
Comment 21•11 years ago
|
||
Since much conflict and unit test fail between other patch merged/revert, I spend much time to merge the pr in master. Finally, it's landed now.
Gaia/master: aa48f672f07fe18c590d31d6d19147193c0269ad
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 22•11 years ago
|
||
thanks!
Comment 23•11 years ago
|
||
Sorry, I accidentally committed some bad changesets to the Gaia repo which you had the misfortunate of pushing on top of. Rather than trying to rebase it all around, I ended up just reverting to the last good changeset, which means your push got lost. I'm *VERY* sorry for the inconvenience I've caused you :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Sorry, I accidentally committed some bad changesets to the Gaia repo which
> you had the misfortunate of pushing on top of. Rather than trying to rebase
> it all around, I ended up just reverting to the last good changeset, which
> means your push got lost. I'm *VERY* sorry for the inconvenience I've caused
> you :(
Hi Ryan, could you please revert your patch? Because the commit log is not correct. There is no log about that
9df14421065bcc5fcb94735f6ab7cb34d6ddc643
aa48f672f07fe18c590d31d6d19147193c0269ad
were reverted. If I create another pull request and merge the it again, the commit log are doubled. It will make it more mixed. How do you think my suggestion here? Thanks.
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 25•11 years ago
|
||
Clean ni flag. I have created a pull request and merged it with Travis test passed.
Gaia/master: 09fe38e11a1979567838bfbadb5122af61ae8dcf
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Comment 26•11 years ago
|
||
Unfortunately, this had to be reverted for Marionette-webapi bustage. I can't say whether this would have happened on the original commit or not due to it landing on the other bustage then.
https://github.com/mozilla-b2g/gaia/commit/796624dc071d5318d7068627ccd2ff7d51436191
https://tbpl.mozilla.org/php/getParsedLog.php?id=36868084&tree=B2g-Inbound
04:48:30 ERROR - errors.JavascriptException: JavascriptException: TypeError: this.states.activeDialog is null
04:48:30 INFO - stacktrace:
04:48:30 INFO - @app://system.gaiamobile.org/js/system_dialog_manager.js, line 197
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 27•11 years ago
|
||
Fixed error from TBPL(https://tbpl.mozilla.org/?tree=Try&rev=1dadff871a04). And Travis passed. We can merge the pr.
gaia/master: fc9aee8c31a7c978319c3ee924947044b5414c9a
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•