Gabriele Svelto [:gsvelto] Assignee
	Description • 11 years ago

Currently URLs contained in a WAP Push message are displayed as plain text. They should be highlighted as links instead and when clicked it should be possible to open them in the browser app.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 11 years ago

Comment on attachment 805614 [details] [diff] [review]
[PATCH] Extract the link from a message and make it clickable

Big patch but mostly trivial so here's a few comment to simplify the review:

- First of all the original scope was to extract the URLs from the messages and be open them in the browser when clicked. This part includes parsing the XML code contained in the messages, filling up the relevant links and handling the clicks via an activity. The only code I wrote from scratch is the first part; the code in link_action_handler.js and activity_picker.js has been taken verbatim from the SMS app and is known to work.

- Prior to this patch I used an attention screen to display the message in the hope that it would make hiding the application easier. As it turned out it made it a nightmare (especially so when activities are involved) so I removed it and used the main application screen to display the messages instead. To hide the application from the cards-view I just close it whenever it's dismissed by the user (i.e. not only when it's closed but also when it gets hidden).

- Finally I did some refactoring to more closely follow our coding conventions and moved the utility codes under utils.js. Those functions have also been taken from the SMS app and are known to work.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 4 • 11 years ago

Pointer to Github pull-request

	Gabriele Svelto [:gsvelto] Assignee
	Comment 5 • 11 years ago

Last minute fix, I realized I had the notifications implemented all wrong which I fixed. All the notes from comment 3 still apply but this has actually simplified the code that dealt with notifications.

BTW testing was done in the emulator following the procedure described in bug 891762 comment 11.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 6 • 11 years ago

Comment on attachment 805990 [details] [diff] [review]
[PATCH v2] Extract the link from a message and make it clickable

I'm busy with e.me related stuff. I suggest that you find an other reviewer. Let's see if the system FE team has some bandwidth for that.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 7 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> I'm busy with e.me related stuff. I suggest that you find an other reviewer.
> Let's see if the system FE team has some bandwidth for that.

I figured that since this contains a bunch of code shared with the SMS app someone from the comms team could help me out with this. Thanks anyway :)

	Gabriele Svelto [:gsvelto] Assignee
	Comment 8 • 11 years ago

Comment on attachment 805990 [details] [diff] [review]
[PATCH v2] Extract the link from a message and make it clickable

Passing the review to Julien since large parts of this patch are made of code coming from the SMS app.

A bit of background on this: WAP Push messages are XML documents delivered via a message handler, they can contain text, URLs and other stuff. What I'm doing in this patch is to extract the URLs from the (pre-validated) message and make it clickable via an activity. The changes consist of the following:

- The presentation part that used to be displayed in an attention screen but proved to be detrimental. All the UI elements have thus been moved to the main app page; one important detail is that the app is always closed when hidden as it needs to behave as some kind of background-ish service and not a real app.

- The message parsing part is very simple as the XML messages have been already validated by Gecko before being delivered. Malformed messages wouldn't be delivered at all making error checking trivial.

- The code used to deal with the browser activity (activity_picker.js and link_action_handler.js) as well as the utilities (utils.js) have been taken from the SMS app and are practically unmodified.

- Finally since URLs are now properly shown in notifications too I've cleaned up the notification code because it had some major problems.

	Julien Wajsberg [:julienw]
	Comment 9 • 11 years ago

Comment on attachment 805990 [details] [diff] [review]
[PATCH v2] Extract the link from a message and make it clickable

Review of attachment 805990 [details] [diff] [review]:
-----------------------------------------------------------------

I admit I didn't tried it. How can I test this ? In the emulator ?

Also I'd like unit tests for this please :)

Please ask me review again when you're ready !

Thanks

::: apps/wappush/index.html
@@ +31,5 @@
>    <body role="application">
> +    <article id="wappush-screen" role="region">
> +      <section role="region">
> +        <header>
> +          <button id="close"><span class="icon icon-close"> close </span></button>

does this "close" need to be translated ? Even if it's not displayed, I think it's read by screen readers

@@ +42,5 @@
> +        <div id="message">
> +          <p id="message-text"></p>
> +          <a id="message-link" data-url="" data-action="url-link"></a>
> +        </div>
> +      </section>

lots of ids here, I wonder if that's not possible to not use most of them.

Like the link could be `document.querySelector('#wappush-container a')` and the text `document.querySelector('#wappush-container p')`.

And if you keep the container in a variable, you could use eg `container.querySelector('a')` which feels very clean.

::: apps/wappush/js/activity_picker.js
@@ +7,5 @@
> +function tryActivity(opts, onsuccess, onerror) {
> +  var activity;
> +
> +  if (typeof onerror !== 'function') {
> +    onerror = function(error) {

please name your anonymous function

@@ +12,5 @@
> +      console.warn('Unhandled error spawning activity; ' + error.message);
> +    };
> +  }
> +
> +  try {

why this try-catch block ?

If this is because MozActivity might be non available, you'd better check that window.MozActivity exists and is truthy

::: apps/wappush/js/link_action_handler.js
@@ +9,5 @@
> +  */
> +  var inProgress = false;
> +
> +  var LinkActionHandler = {
> +    onClick: function lah_onClick(event) {

please call |event.preventDefault();|  maybe not strictly necessary but before be safe than sorry.

@@ +26,5 @@
> +      }
> +
> +      inProgress = true;
> +
> +      type = action.replace('-link', '');

it seems to me that your action could not contain '-link' from the start ?

@@ +37,5 @@
> +        dataset[type], this.reset, this.reset
> +      );
> +    },
> +
> +    reset: function lah_reset() {

cool, this function will make it easy to write unit tests ;)

::: apps/wappush/js/wappush.js
@@ +18,5 @@
> + *
> + * @param {Object} message A WAP Push message as delivered by the system.
> + */
> +
> +function ParsedMessage(message) {

maybe I'm being an object oriented ayatollah here, but this looks like a factory for me.

How about a "ParsedMessage.from(message)" that would return a ParsedMessage object, instead of calling directly a constructor ? This would also allow you to return null in case the content-type is not recognized (see below).

Then the (logically private, but you don't need to make it really private) ParsedMessage constructor would just take an option object to populate its properties.

This would replace your "shouldDisplayMessage" function by a more OO approach.

@@ +22,5 @@
> +function ParsedMessage(message) {
> +  var parser = new DOMParser();
> +  var doc = parser.parseFromString(message.content, 'text/xml');
> +
> +  // We don't check for errors as the message has already been validated

where is it validated ?

@@ +24,5 @@
> +  var doc = parser.parseFromString(message.content, 'text/xml');
> +
> +  // We don't check for errors as the message has already been validated
> +
> +  this.type = (message.contentType == 'text/vnd.wap.si') ? 'si' : 'sl';

is 'sl' associated to a specific contentType too ? Might be a good idea to white list all possible content types.

and do this before parsing the markup :)

maybe you could set this property inside the next if-block instead ?

@@ +28,5 @@
> +  this.type = (message.contentType == 'text/vnd.wap.si') ? 'si' : 'sl';
> +
> +  if (message.contentType == 'text/vnd.wap.si') {
> +    // SI message
> +    var indicationNode = doc.getElementsByTagName('indication')[0];

`doc.querySelector('indication')` works as well and is shorter.

@@ +31,5 @@
> +    // SI message
> +    var indicationNode = doc.getElementsByTagName('indication')[0];
> +
> +    this.href = indicationNode.getAttribute('href');
> +    this.text = indicationNode.innerHTML;

why using innerHTML and not textContent ? Do we really want to keep the markup ?

@@ +32,5 @@
> +    var indicationNode = doc.getElementsByTagName('indication')[0];
> +
> +    this.href = indicationNode.getAttribute('href');
> +    this.text = indicationNode.innerHTML;
> +  } else {

same "white list" comment than before

@@ +34,5 @@
> +    this.href = indicationNode.getAttribute('href');
> +    this.text = indicationNode.innerHTML;
> +  } else {
> +    // SL message
> +    var slNode = doc.getElementsByTagName('sl')[0];

`doc.querySelector('sl')` is shorter :)

@@ +93,5 @@
> +    this._link = document.getElementById('message-link');
> +
> +    // Event handlers
> +    document.addEventListener('visibilitychange',
> +                              this.onVisibilityChange.bind(this));

nit: I usually write multiple lines commands like that:

  document.addEventListener(
    'visibilitychange',
    this.onVisibilityChange.bind(this)
  );

But this is merely a suggestion, do what you want here, this is not something I want to enforce.

@@ +164,4 @@
>        return;
>      }
>  
> +    var contents = new ParsedMessage(message);

you should get a ParsedMessage object in the setItem callback as you don't use it before that.

@@ +174,4 @@
>           * retrieve the message from the notification code */
>          var iconURL = NotificationHelper.getIconURI(app) +
>                        '?timestamp=' + encodeURIComponent(timestamp);
> +        var text = (contents.text !== undefined) ? (contents.text + ' ') : '';

what about:

  var text = contents.text ? contents.text + ' ' : '';

I believe you don't want to add a space if `contents.text` is the empty string :)

@@ +201,5 @@
> +    if (!message.clicked) {
> +      return;
> +    }
> +
> +    var params = Utils.deserializeParameters(message.imageURL);

you can deserialize the params inside the onsuccess handler.

Having less variables captures by a closure is better.

@@ +223,5 @@
> +
> +      // Populate the message
> +      this._title.innerHTML = message.sender;
> +      this._text.innerHTML = Utils.escapeHTML(contents.text);
> +      this._link.innerHTML = contents.href;

why don't you use textContent instead of innerHTML here ?

And then no need to use escapeHTML :)

@@ +224,5 @@
> +      // Populate the message
> +      this._title.innerHTML = message.sender;
> +      this._text.innerHTML = Utils.escapeHTML(contents.text);
> +      this._link.innerHTML = contents.href;
> +      this._link.dataset.url = contents.href;

I think you should put the href as the link's href too.

In the onclick handler we'll preventDefault anyway :)

@@ +239,2 @@
>     * Closes the application, lets the event loop run once to ensure clean
>     * termination of pending events.

This doesn't sound very reliable.

Maybe it's better to use a boolean like "canClose" or "eventsInProgress" that you would set in those events, and another boolean "willClose" that you would set here, and will be checked in the various event handlers.

But it's up to you and anyway this should be done in another patch.

::: apps/wappush/style/wappush.css
@@ +31,5 @@
>    word-wrap: break-word;
>  }
> +
> +#message a {
> +  text-decoration: underline;

you don't need this if you put an `href` to your link

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 11 years ago

First of all thank you for your extensive review, for me this is a crash-course in HTML5/JS/CSS best practices :) Just a couple of inline comments:

(In reply to Julien Wajsberg [:julienw] from comment #9)
> I admit I didn't tried it. How can I test this ? In the emulator ?

Yes, which is not very convenient.

> Also I'd like unit tests for this please :)

I will but I hoped you wouldn't notice ;)


> lots of ids here, I wonder if that's not possible to not use most of them.
> 
> Like the link could be `document.querySelector('#wappush-container a')` and
> the text `document.querySelector('#wappush-container p')`.
> 
> And if you keep the container in a variable, you could use eg
> `container.querySelector('a')` which feels very clean.

Sounds good but is there anything inherently wrong in using IDs for unique elements?

> why this try-catch block ?

I don't know, I've copy-pasted this code from the SMS app :)

> it seems to me that your action could not contain '-link' from the start ?

Yeah, since I've taken all of this functionality from the SMS app I didn't want to change it too much because I knew the original code worked already. I will clean it up though, this is really useless here.

> How about a "ParsedMessage.from(message)" that would return a ParsedMessage
> object, instead of calling directly a constructor ? This would also allow
> you to return null in case the content-type is not recognized (see below).
>
> Then the (logically private, but you don't need to make it really private)
> ParsedMessage constructor would just take an option object to populate its
> properties.

Sounds good :) OO-paradigms in JavaScript still elude me a bit.

> where is it validated ?

In Gecko; the code that handles the messages verifies them against their DTDs.

> why using innerHTML and not textContent ? Do we really want to keep the
> markup ?

No, it's just that I didn't know the difference until now :p

> This doesn't sound very reliable.

Indeed, I'm fiddling with some follow up patches too and I've found a better way to handle it.

I'll address all your other comments in a new patch and ask for review again.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 11 • 11 years ago

Updated patch with all everything from comment 9 addressed except for two things:

- I haven't chopped out the '-link' part when handling highlighted links because this will make it easier to extend this mechanism in the future just as it is used in the SMS app. It doesn't hurt either way IMHO.

- I'm not storing the raw messages but instead I'm storing the parsed one. This saves one step of parsing when displaying the message and also paves the way for the changes I have to do in other bugs.

I'm currently writing the unit-tests, I'll ask for review once I've got those ready.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 12 • 11 years ago

Updated patch now complete with unit tests. I'll file a follow-up bug to address the unit tests for the functionality that's already there but not covered already.

	Julien Wajsberg [:julienw]
	Comment 13 • 11 years ago

Comment on attachment 808566 [details] [diff] [review]
[PATCH v4] Extract the link from a message and make it clickable

Gabriele told me he would provide an updated patch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 14 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Comment on attachment 808566 [details] [diff] [review]
> [PATCH v4] Extract the link from a message and make it clickable
> 
> Gabriele told me he would provide an updated patch.

Yep, coming right away.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 15 • 11 years ago

Brand new patch now with tests attached :) The patch is larger and contains some extra changes which I've pulled in from follow-ups because I also wanted to add more tests and those depended on changes I had already made. So in order to not re-review stuffs twice I've folded those changes in this patch.

Anyway in this patch I've address all your nits from your previous review save for these extra changes:

- I haven't chopped out the '-link' part when handling highlighted links because this will make it easier to extend this mechanism in the future just as it is used in the SMS app. It doesn't hurt either way IMHO.

- I'm not storing the raw messages but instead I'm storing the parsed ones and I'm using a dedicated IndexedDB store instead of asyncStorage. This paves the way for the future patches and allows me to fix an issue I had before: when displaying a message I wanted to remove it from the storage but this operation wasn't atomic and so it was subject to races. Now the operation is atomic.

- I've split out message parsing and utility functions.

- I've added tests for most of the functionality - not only the new one - and proper mocks to support them.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 16 • 11 years ago

Sorry for the last minute update but while testing I noticed that I had a left a piece of broken functionality in this patch that really belonged to a follow-up. I just chopped out a few lines from parsed_message.js so if you started reviewing this it shouldn't make much of a difference.

	Julien Wajsberg [:julienw]
	Comment 17 • 11 years ago

Comment on attachment 810621 [details] [diff] [review]
[PATCH v5] Extract the link from a message and make it clickable

Review of attachment 810621 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't finished my review yet (esp the tests), will do monday, but here are the main points in the actual code.

::: apps/wappush/js/activity_picker.js
@@ +10,5 @@
> +  if (typeof onerror !== 'function') {
> +    onerror = function defaultError(event) {
> +      console.warn('Unhandled error spawning activity; ' +
> +                   event.target.error.message);
> +    };

please define this function outside of tryActivity

@@ +21,5 @@
> +      activity.onsuccess = onsuccess;
> +    }
> +
> +    activity.onerror = onerror;
> +  }

what do we do if there is no MozActivity ? just do nothing ? (might be ok, I don't know :) )

::: apps/wappush/js/messagedb.js
@@ +10,5 @@
> +  var KEY_PATH = 'timestamp';
> +  var ID_INDEX = 'id';
> +  var ID_INDEX_KEYPATH = 'id';
> +
> +  var db = null;

I'd rather not keep a shared db, I'd rather open the db each time you need it, and passing the db as parameter to your callbacks

::: apps/wappush/js/parsed_message.js
@@ +2,5 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +
> +'use strict';
> +
> +var ParsedMessage = (function() {

so, instead of having a MessageDB, I'd suggest that you do this:

* convert MessageDB to an IndexedDBHelper that could be put in shared, this is already quite close to being generic (kind of like we have in Gecko after all, but without the inheritance part)
* make ParsedMessage a little more object oriented, like this:

  function ParsedMessage(obj) { if(obj) { loop to copy obj properties to this } }

  ParsedMessage.prototype = {
    save: function() {
      // save in indexeddb using the indexeddb helper
    }
  };

  ParsedMessage.from = function pm_from() { // keep current from implem, but do "var obj = new ParsedMessage();" instead of "var obj = {}" }
  ParsedMessage.load = function pm_load(timestamp} { // retrieve from db usinfg indexeddb helper, and return new ParsedMessage(state) }

and export ParsedMessage to window using a closure like:

(function(exports) {
  ... previous code

  exports.ParsedMessage = ParsedMessage;
})(window);

@@ +46,5 @@
> +      obj.text = indicationNode.textContent;
> +
> +      // 'si-id' attribute, optional, string
> +      if (indicationNode.hasAttribute('si-id')) {
> +        this.id = indicationNode.getAttribute('si-id');

I think you want obj.id ?

@@ +47,5 @@
> +
> +      // 'si-id' attribute, optional, string
> +      if (indicationNode.hasAttribute('si-id')) {
> +        this.id = indicationNode.getAttribute('si-id');
> +      } else if (this.href) {

same here: obj.href

@@ +50,5 @@
> +        this.id = indicationNode.getAttribute('si-id');
> +      } else if (this.href) {
> +        /* WAP-167 5.2.1: If the 'si-id' attribute is not specified, its value
> +         * is considered to be the same as the value of the 'href' attribute */
> +        this.id = this.href;

same here: obj.id/obj.href

@@ +51,5 @@
> +      } else if (this.href) {
> +        /* WAP-167 5.2.1: If the 'si-id' attribute is not specified, its value
> +         * is considered to be the same as the value of the 'href' attribute */
> +        this.id = this.href;
> +      }

if we have no id, what happens ? does the Earth explode ?

@@ +55,5 @@
> +      }
> +
> +      // 'created' attribute, optional, date in ISO 8601 format
> +      if (indicationNode.hasAttribute('created')) {
> +        this.created = Date.parse(indicationNode.getAttribute('created'));

obj.created

	Gabriele Svelto [:gsvelto] Assignee
	Comment 18 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #17)
> I haven't finished my review yet (esp the tests), will do monday, but here
> are the main points in the actual code.

If you wait a little bit I'll refresh the patches again with your suggestions here. I've also posted follow-up patches on other bugs, I'll cancel reviews there because those should be looked at only once I'm done finalizing this (though the changes shouldn't be very large).

> please define this function outside of tryActivity

OK.

> what do we do if there is no MozActivity ? just do nothing ? (might be ok, I
> don't know :) )

Yeah, I think that's what other applications are doing too.

> I'd rather not keep a shared db, I'd rather open the db each time you need
> it, and passing the db as parameter to your callbacks

I can do that but why would it be needed? My idea was to lazily open the db but re-use it in following calls to save on IPC with the main process (and related operations).

> so, instead of having a MessageDB, I'd suggest that you do this:
> 
> * convert MessageDB to an IndexedDBHelper that could be put in shared, this
> is already quite close to being generic (kind of like we have in Gecko after
> all, but without the inheritance part)
> * make ParsedMessage a little more object oriented, like this:
> 
>   function ParsedMessage(obj) { if(obj) { loop to copy obj properties to
> this } }
> 
>   ParsedMessage.prototype = {
>     save: function() {
>       // save in indexeddb using the indexeddb helper
>     }
>   };
> 
>   ParsedMessage.from = function pm_from() { // keep current from implem, but
> do "var obj = new ParsedMessage();" instead of "var obj = {}" }
>   ParsedMessage.load = function pm_load(timestamp} { // retrieve from db
> usinfg indexeddb helper, and return new ParsedMessage(state) }
> 
> and export ParsedMessage to window using a closure like:
> 
> (function(exports) {
>   ... previous code
> 
>   exports.ParsedMessage = ParsedMessage;
> })(window);

OK, will do.

> I think you want obj.id ?
> [...]

Yeah, these bits and pieces shouldn't have been in the patch in the first place. They belong to the follow-up, sorry for the inconvenience.

> if we have no id, what happens ? does the Earth explode ?

id-less messages are supported, they just don't go through out-of-order management, updates and so. In fact this first patch shouldn't bother with that part so I've chopped this out.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 19 • 11 years ago

New patch, I've addressed all your comments except for the generalization of MessageDB. I've given it some thought and it wouldn't work very well here because I'm implementing in follow-ups some complex operations which will happen within the DB and are not really generic. For example the 'put' method will start checking message indexes, creation times and expiration times and according to these parameters will drop, update or store messages conditionally. Since all these operations need to happen in a single atomic transaction all this code will have to live in the database.

Additionally the clock app is already exposing a fairly generic IndexedDB helper and we might consider putting that one into shared instead, see:

http://is.gd/dnoTib

	Julien Wajsberg [:julienw]
	Comment 20 • 11 years ago

I'm sorry Gabriele, I don't follow the problem of generalizing a very simple DB Helper with complex operations ?

The generic helper in clock could be reused (some parts are not generic yet, eg the version part in the open call :) )

Let's see your current code already.

	Julien Wajsberg [:julienw]
	Comment 21 • 11 years ago

Comment on attachment 812573 [details] [diff] [review]
[PATCH v7] Extract the link from a message and make it clickable

Review of attachment 812573 [details] [diff] [review]:
-----------------------------------------------------------------

still couldn't review everything but you have the most important here.

::: apps/wappush/js/messagedb.js
@@ +46,5 @@
> +      return;
> +    }
> +
> +    req.onsuccess = function mdb_opened(event) {
> +      db = event.target.result;

FYI I checked with bent and it's actually better to cache the db :)

@@ +53,5 @@
> +    req.onerror = function mdb_openError(event) {
> +      error(event.target.errorCode);
> +    };
> +    req.onupgradeneeded = function mdb_upgradeNeeded(event) {
> +      db = event.target.result;

let's keep that db local. `onsuccess` will be called anyway.

@@ +85,5 @@
> +        error(event.target.errorCode);
> +      };
> +
> +      var store = transaction.objectStore(MESSAGE_STORE_NAME);
> +      store.put(message);

The idiom is more like:

  var transaction = db.transaction(MESSAGE_STORE_NAME, 'readwrite');
  var store = transaction.objectStore(MESSAGE_STORE_NAME);
  var request = store.put(message);
  request.onsuccess = function() { success() };

but your approach should work (checked with bent :) ).


However, also checked with bent, I think we need to convert the message to a JSON object. So let's do something like:

  if (message.toJSON) {
    message = message.toJSON();
  }
  store.put(message);

@@ +123,5 @@
> +        if (event.target.result) {
> +          var message = event.target.result;
> +
> +          state.message = message;
> +          store.delete(message.timestamp);

ok, this part is clearly not generic ;)

I wonder if you could not call a callback instead of deleting here ?

Might be for a follow-up bug though.

@@ +127,5 @@
> +          store.delete(message.timestamp);
> +        }
> +      };
> +    } else {
> +      mdb_open(mdb_retrieve.bind(this, timestamp, success, error), error);

I wonder if we could not make mdb_open call the callback asap if db already exists.

That way all your functions would be cleaner: you would just always call mdb_open.

::: apps/wappush/js/parsed_message.js
@@ +14,5 @@
> +      var i;
> +
> +      for (i in obj) {
> +        this[i] = obj[i];
> +      }

nit: I'd rather use "key" instead of "i". and you can use "var" directly inside the parens:

for (var key in obj) {
  this[key] = obj[key];
}

@@ +22,5 @@
> +  ParsedMessage.prototype = {
> +    /**
> +     * Constructor, this is for internal use only
> +     */
> +    constructor: ParsedMessage,

you don't use this (yet) ?

@@ +35,5 @@
> +     * @param {Function} error A callback invoked if an operation fails.
> +     */
> +    save: function pm_save(success, error) {
> +      MessageDB.put(this, success, error);
> +    }

add a toJSON function that would generate an untyped object from your typed object.

@@ +104,5 @@
> +   */
> +  ParsedMessage.load = function pm_load(timestamp, success, error) {
> +    MessageDB.retrieve(timestamp,
> +      function pm_loadSuccess(message) {
> +        if (message != null) {

"if (message)" is probably enough

::: apps/wappush/test/unit/activity_picker_test.js
@@ +14,5 @@
> +
> +  suiteSetup(function() {
> +    realMozL10n = navigator.mozL10n;
> +    navigator.mozL10n = MockL10n;
> +    mocksHelperAP.suiteSetup();

just call "mocksHelperAP.attachTestHelpers()" outside of suiteSetup() and suiteTeardown() and remove the "mocksHelperAP.suiteSetup()", "mocksHelperAP.suiteTeardown()", etc lines. Same for other test files.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 22 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Comment on attachment 812573 [details] [diff] [review]
> [PATCH v7] Extract the link from a message and make it clickable
> 
> Review of attachment 812573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> still couldn't review everything but you have the most important here.

Thanks, updated patch incoming :)

> FYI I checked with bent and it's actually better to cache the db :)

OK, I'll keep it this way then.

> let's keep that db local. `onsuccess` will be called anyway.

Will do.

> The idiom is more like:
> 
>   var transaction = db.transaction(MESSAGE_STORE_NAME, 'readwrite');
>   var store = transaction.objectStore(MESSAGE_STORE_NAME);
>   var request = store.put(message);
>   request.onsuccess = function() { success() };
> 
> but your approach should work (checked with bent :) ).

Well, both approaches are supported, they just have different meanings. In your example you're waiting for the request to complete, in mine  I care about the transaction. Since I don't need to do anything after the last operation I don't care about that particular request and only want to be notified once the transaction is finished. This makes it easier to extend the transaction as I won't have to move the "I'm finished" code around.

> However, also checked with bent, I think we need to convert the message to a
> JSON object. So let's do something like:
> 
>   if (message.toJSON) {
>     message = message.toJSON();
>   }
>   store.put(message);

Yeah, it's not needed but it's more efficient. Storing objects in the DB directly is supported but it means that the entire objects are stored - methods and all - which is not such a good idea :)

> ok, this part is clearly not generic ;)
> 
> I wonder if you could not call a callback instead of deleting here ?
> 
> Might be for a follow-up bug though.

This part becomes more complex in the follow-ups with the transaction becoming a complex series of operations. The issue here is that the WAP Push specification mandates a specific flow for handling incoming messages. You can see it here on page 5:

http://is.gd/3jJNcf

I'm implementing this flow one step at a time for ease of review. Breaking it up in multiple transactions (like it was in my original patch) means that the database can be left in an inconsistent state. Using a single transaction on the other hand guarantees atomic completion of the operations so they're all done or none of them are.

> I wonder if we could not make mdb_open call the callback asap if db already
> exists.
> 
> That way all your functions would be cleaner: you would just always call
> mdb_open.

Yup, I'm changing all my code to something like:

  function mdb_open(success, error) {
    if (db) {
      success(db);
      return;
    }

    // Rest of the stuff ...
  }

  function mdb_put(message, success, error) {
    mdb_open(function mdb_putCallback(db) {
      var transaction = db.transaction(MESSAGE_STORE_NAME, 'readwrite');
      // Rest of the stuff ...
    }
  }

This gets rid of the if (db) { ... } else { mdb_open(...); } flow.

> nit: I'd rather use "key" instead of "i". and you can use "var" directly
> inside the parens:
> 
> for (var key in obj) {
>   this[key] = obj[key];
> }

OK.

> you don't use this (yet) ?

Not yet, I've put it in for completeness.

> add a toJSON function that would generate an untyped object from your typed
> object.

Will do.

> "if (message)" is probably enough

OK.

> just call "mocksHelperAP.attachTestHelpers()" outside of suiteSetup() and
> suiteTeardown() and remove the "mocksHelperAP.suiteSetup()",
> "mocksHelperAP.suiteTeardown()", etc lines. Same for other test files.

OK, makes things easier, thanks.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 23 • 11 years ago

Updated patch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 24 • 11 years ago

Un-bitrotted the patch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 25 • 11 years ago

This patch makes the WAP Push application visible and adds a test button that triggers a message. It can be used to test the app in the desktop version without having to use the emulator. The 'test' button can be clicked up to three times to send fake messages, afterwards one needs to restart the app. The first two messages contain only text, the third also a link.

	Julien Wajsberg [:julienw]
	Comment 26 • 11 years ago

Comment on attachment 814826 [details] [diff] [review]
[PATCH v9] Extract the link from a message and make it clickable

r=me

I really think we'll need to revisit how we do complex indexed db operations, but let's do that in another patch.

Thanks for this work !

	Gabriele Svelto [:gsvelto] Assignee
	Comment 27 • 11 years ago

(In reply to Julien Wajsberg [:julienw] (in MozSummit until next monday) from comment #26)
> r=me

Thank you for this review and sorry for the size of it!

> I really think we'll need to revisit how we do complex indexed db
> operations, but let's do that in another patch.

OK, there's going to be some refactoring happening later anyway. I want to streamline the unit-tests too for example.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 28 • 11 years ago

Merged to gaia master 7b80daa4f61cd04dd8b3241024a1d42e37666742

This need uplifting to 1.2. I'll check if the patch applies cleanly and if it doesn't I'll upload an updated patch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 29 • 11 years ago

Cherry picking does not work right away due to a small change between master and v1.2 so here's the uplifted patch for gaia/v1.2.

	Enpei
	Comment 30 • 11 years ago

Hi Gabriele,

v1.2 doesn't has the fix. And from comment 29, will the patch needs to be reviewed again before land on 1.2? When will this be solved? This is an koi+ issue and I need to verify it on v1.2.

Buri v1.2 still has the bug:
Gaia:     d0f0110f6a907ef386134dc693f5dc5a632a0e9e
Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/818ef9b4096c
BuildID   20131013004004
Version   26.0a2

	Gabriele Svelto [:gsvelto] Assignee
	Comment 31 • 11 years ago

(In reply to Enpei from comment #30)
> And from comment 29, will the patch needs to be reviewed again before land on 1.2?

I don't think it doesn't as there's only a one-line change compared to master so I assume it can land as r=julienw as the master patch.

> When will this be solved? This is an koi+ issue and I need to verify it on v1.2.

I've set the flags for uplifting it and I was waiting for the sheriffs but if you need it ASAP I can uplift it myself.

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 32 • 11 years ago

We haven't been forcing re-review on uplift conflicts for anything else as long as they don't materially change the patch.

v1.2: ee288b820a42c7a156a7a6cc392f35899dbfdf20

	Julien Wajsberg [:julienw]
	Comment 33 • 11 years ago

Gabriele> you could have merged it yourself too, you know :)

	Gabriele Svelto [:gsvelto] Assignee
	Comment 34 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #33)
> Gabriele> you could have merged it yourself too, you know :)

Will do it next time, lazy me :-P

	Enpei
	Comment 35 • 11 years ago

Verified on Buri 1.2
Gaia:     04ee9e4430b25ba2c38752d3897f0ee5e2a6ab80
Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/52f24889dccc
BuildID   20131027004003
Version   26.0a2