Bug 853798 - [Buri][Dialer]The screen display abnormal after using USSD #9062

Merged
merged 0 commits into from Apr 15, 2013

Projects

None yet

3 participants

@rik
rik commented Apr 9, 2013
  • Refactored the USSD UI to not use a new window but only a regular DOM element
@etiennesegonzac
Collaborator

General comments:

  • it looks good (the code)
  • it works very well
  • I think keeping the postMesage was the right choice, it makes the change very clear

Now less fun, we have a unique opportunity to fix bug 799435 [1] and I think we should take it (and also rename the files).
So my inline comments will be based on the fact that we're mmi-ying all the things [2].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=799435
[2] insert a meme here

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -198,6 +202,40 @@ <h1 id="header-edit-mode-text" data-l10n-id="edit">Edit</h1>
<button>Action 2</button>
</menu>
</form>
+
+ <article id="message-screen" role="region" hidden>
+ <section role="region">
+ <header>
+ <button id="message-close"><span class="icon icon-close"> close </span></button>
+ <menu type="toolbar">
+ <button id="send" disabled> send </button>
+ </menu>
+ <h1 id="header-title">USSD</h1>
+ </header>
+ </section>
+ <section id="message-container" role="region">
+ <div id="message">Wesh
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

wesh gros, bien ou bien ? on cleanup un peu le markup avant de lander ?

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/dialer.js
@@ -341,7 +341,12 @@ var CallHandler = (function callHandler() {
/* === USSD === */
function init() {
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

nit: we should rename the function for initMMI since it's just doing mmi stuffs.

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd.js
@@ -25,6 +22,12 @@ var UssdManager = {
if (this._conn.voice) {
this._conn.addEventListener('voicechange', this);
this._operator = MobileOperator.userFacingInfo(this._conn).operator;
+ var message = {
+ type: 'voicechangeUI',
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

how about a mmi-* prefix for the messages? since the whole dialer app frame is getting the message I'd like to be more specific.

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd.js
@@ -138,106 +139,25 @@ var UssdManager = {
type: 'error',
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-error ?

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd.js
- // even before the new USSD window has been opened and/or
- // initialized.
- this._popup.addEventListener('ready', this.uiReady.bind(this));
-
- if (!ussd) {
- return;
- }
- // The message containing the received USSD won't be delivered until
- // the UI notifies about its successfull load.
- var message = {
- type: 'ussdreceived',
- message: ussd.message,
- sessionEnded: ussd.sessionEnded
- };
- this.postMessage(message, this.COMMS_APP_ORIGIN);
+ window.postMessage({type: 'loading'}, this.COMMS_APP_ORIGIN);
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-loading ?

@etiennesegonzac etiennesegonzac and 1 other commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd.js
@@ -249,7 +169,7 @@ var UssdManager = {
// Do not notify the UI if no message to show.
if (evt.message != null || evt.sessionEnded)
message = {
- type: 'ussdreceived',
+ type: 'ussdreceivedUI',
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-received

@rik
rik Apr 10, 2013

I prefer mmi-received-ui to not be confused with ussdreceived that might get renamed to 'mmireceived' one day.

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd.js
@@ -257,7 +177,7 @@ var UssdManager = {
case 'voicechange':
this._operator = MobileOperator.userFacingInfo(this._conn).operator;
message = {
- type: 'voicechange',
+ type: 'voicechangeUI',
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

tricky one, I'd say mmi-networkchange

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd_ui.js
},
- cancel: function uui_cancel() {
- window.opener.postMessage({
+ closeWindow: function uui_closeWindow() {
+ window.postMessage({
type: 'cancel'
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-cancel

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/js/ussd_ui.js
@@ -137,7 +133,7 @@ var UssdUI = {
reply: function uui_reply() {
this.showLoading();
var response = this.responseTextNode.value;
- window.opener.postMessage({
+ window.postMessage({
type: 'reply',
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-reply

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/test/unit/mock_ussd_ui.js
@@ -7,18 +7,22 @@ var MockUssdUI = {
_messageReceived: null,
_sessionEnded: null,
- postMessage: function muui_postMessage(message, origin) {
- switch (message.type) {
- case 'ussdreceived':
- this._messageReceived = message.message;
- this._sessionEnded = message.sessionEnded;
+ postMessage: function muui_postMessage(evt) {
+ if (!evt.data) {
+ return;
+ }
+
+ switch (evt.data.type) {
+ case 'ussdreceivedUI':
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

yep, definitely, it will look much better after the event names rename.

@etiennesegonzac etiennesegonzac and 1 other commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -198,6 +202,40 @@ <h1 id="header-edit-mode-text" data-l10n-id="edit">Edit</h1>
<button>Action 2</button>
</menu>
</form>
+
+ <article id="message-screen" role="region" hidden>
+ <section role="region">
+ <header>
+ <button id="message-close"><span class="icon icon-close"> close </span></button>
+ <menu type="toolbar">
+ <button id="send" disabled> send </button>
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-send ?

@rik
rik Apr 10, 2013

We can't change the ID because of L10N strings.

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -198,6 +202,40 @@ <h1 id="header-edit-mode-text" data-l10n-id="edit">Edit</h1>
<button>Action 2</button>
</menu>
</form>
+
+ <article id="message-screen" role="region" hidden>
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

let's mmify the markup too, message could lead to confusion in a (sick) Communications app.

-> mmi-screen

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -198,6 +202,40 @@ <h1 id="header-edit-mode-text" data-l10n-id="edit">Edit</h1>
<button>Action 2</button>
</menu>
</form>
+
+ <article id="message-screen" role="region" hidden>
+ <section role="region">
+ <header>
+ <button id="message-close"><span class="icon icon-close"> close </span></button>
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-close

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -198,6 +202,40 @@ <h1 id="header-edit-mode-text" data-l10n-id="edit">Edit</h1>
<button>Action 2</button>
</menu>
</form>
+
+ <article id="message-screen" role="region" hidden>
+ <section role="region">
+ <header>
+ <button id="message-close"><span class="icon icon-close"> close </span></button>
+ <menu type="toolbar">
+ <button id="send" disabled> send </button>
+ </menu>
+ <h1 id="header-title">USSD</h1>
+ </header>
+ </section>
+ <section id="message-container" role="region">
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

mmi-container

@etiennesegonzac etiennesegonzac commented on an outdated diff Apr 10, 2013
apps/communications/dialer/index.html
@@ -49,6 +52,7 @@
<script defer src="/dialer/js/telephony_helper.js"></script>
<script defer src="/dialer/js/ussd.js"></script>
<script defer src="/dialer/js/newsletter_manager.js"></script>
+ <script defer src="/dialer/js/ussd_ui.js"></script>
@etiennesegonzac
etiennesegonzac Apr 10, 2013 collaborator

nit: let's group the ussd.js line and the ussd_ui.js lines.

Wait.

I meant mmi.js and mmi_ui.js :)

@rik
rik commented Apr 10, 2013

all-the-things-meme-generator-mmi-all-the-things-aa7a72

@rik
rik commented Apr 10, 2013

I also removed listening to the 'close' message type in mmi_ui.js because we don't send that kind of messages anymore.

@etiennesegonzac etiennesegonzac and 1 other commented on an outdated diff Apr 12, 2013
apps/communications/dialer/js/mmi.js
@@ -19,12 +16,18 @@ var UssdManager = {
// session.
_pendingRequest: null,
@etiennesegonzac
etiennesegonzac Apr 12, 2013 collaborator

Correct me if I'm wrong, but we're not using the _pendingRequest anywhere right?

@dominickuo dominickuo merged commit 2abd232 into mozilla-b2g:master Apr 15, 2013

1 check failed

Details default The Travis build failed
@rik rik deleted the rik:ussd-refactoring-853798 branch Apr 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment