924170 - [Video] Add Marionette tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Punam Dahiya [:pdahiya]]

Add Marionette tests for Video App.

Comment 1

[Punam Dahiya [:pdahiya]]

Attachment: Pull requests for video info integration test

Comment 2

[Punam Dahiya [:pdahiya]]

Test covered in attached PR are:
1. Load video in video gallery
2. Select video in gallery view
3. Play video and click on video info icon to display video info screen.

The marionette test for video app uses module test_common.js written as part of Bug 915661. I have made slight changes to test_common.js e.g. passing mediaType in prepareTestSuites to make it common across all media apps. Ideally test_common.js should be placed outside in a shared folder /shared/test/marionette or shared/js?

Comment 3

[James Lal [:lightsofapollo]]

Sorry for the delay- I will take a detailed look later in the day.

I suspect what we really need to do is isolate the common parts of test_common.js and make it into a standalone project. cc'ing some folks who know about that.

Comment 4

[Mike Pennisi [:jugglinmike]]

I think a file helper might make sense, but it would be a pretty minimal wrapper around the Node.js file API. (For instance, `deleteFile` is essentially an alias for `fs.unlink`.) It would probably only make sense to collect methods that compose more than one function call. Maybe `cleanDirectory` and `makeTmpDir`?

If we go in this direction, we'll want to implement a consistently-synchronous interface.

Comment 5

[James Lal [:lightsofapollo]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Passing to mike

Comment 6

[Mike Pennisi [:jugglinmike]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Punam! I left in-line comments on the GitHub pull request, but I have some
high-level feedback for you, as well:

---

From what I can tell, your intention is to introduce a Marionette test suite
for the application, but you aren't necessarily trying to exhaustively test the
entire application. I think this is a fine approach, but I also think we should
be more explicit with the test organization so that it is more clear what parts
of the application have yet to be tested. I left a comment to this effect on
the pull request, but we should be very specific with the names of the test
files and the test suites so that the current level of coverage is more
obvious.

---

This may sound surprising to you, but we are trying to avoid asynchronous
control flow in the Gaia Marionette tests. This is because a given process's
tests will always run in series, so there is no benefit from doing non-blocking
work. (For instance, even though you have correctly defined this setup function
to be asynchronous, nothing else will be done until it resolves.) Given this,
we should feel free to maintain a simpler, more linear control flow using
synchronous APIs.

---

As for the `Video` object: I think that this is a good start, but that it needs
to be expanded. This took me a little while to understand*, but here is how we
ought to think about it:

Each application's integration test suite should define a sort of "driver"
object that abstracts the details of working within the application. Ideally,
the tests themselves should not refer to specific elements, specific gestures,
or even the Marionette API itself. If you're anything like me, you're probably
thinking, "That sounds like a waste of time. These are just tests, and I need
to get back to improving the application." Here are the benefits to this
approach:

1. The test files are less volatile. This makes reviewing changes much easier.
   Sometimes, changes in the application will require changing certain details
   about the integration tests (maybe a CSS class name is modified, for
   instance). When the code changes like this, it is easier for reviewers to
   see that the high-level application behavior is intact when the integration
   test files themselves are not modified.
2. Succinctness/readability. By expressing the tests in terms of functions that
   give a user-centered name to a technical operation, the tests become more
   concise and easier to understand. For instance, consider the difference
   between a test like this:

      test('should let you select video', function() {
        // Use the 'selection' button and select the test video.
        // Selected video should have an border/highlight.
        app.thumbnail.click();

        var value = client.executeScript(function() {
          var element = document.getElementsByClassName('thumbnail')[0];
          return window.getComputedStyle(element, null).outlineColor;
        });
        assert.ok(value.length > 0);
      });

   ...with a version re-written to encapsulate implementation-specific details:

      test('video selection', function() {
        app.selectThumbnail(1);
        assert.ok(app.isVideoSelected(1));
      });

3. Reusability. If it takes a lot of code to interpret the application status
   in the way that a user would, then it's good to define that code as a
   function. This will allow future regression tests to re-use the same logic
   in scenarios we might be missing today. For example, imagine if we wanted to
   add a test that ensured that more than one video couldn't be selected at the
   same time. Given the above example, we already have a method to express if
   a given video is selected--we just have to call it again in the new test.

* - You can compare my first and current patch for bug 907177: the first is
attachment 792826 [details] [diff] [review] and the current is attachment 815108 [details] [diff] [review]

---

Let me know if any of this is confusing!

Comment 7

[Tom Herold]

If you really want an synchronous implementation for this and go ahead and change, (although I rather prefer the async one).

@Mike Why did you have an mind when you talk about "FileHelper"? Should this module be the basis for that? Is anyone already working on this?

Comment 8

[James Lal [:lightsofapollo]]

So lets try to land this first iteration assuming Tom (or someone else) can take responsibility for building out the standalone module for this (like we do for marionette-forms, marionette-apps, etc..)

Comment 9

[Punam Dahiya [:pdahiya]]

(In reply to Mike Pennisi [:jugglinmike] from comment #6)
> Comment on attachment 814175 [details] [review]
> Pull requests for video info integration test
> 
Thanks Mike for your review, will update PR with your feedback.

Comment 10

[Punam Dahiya [:pdahiya]]

Tried to use marionette-file-manager API (bug 936371) in video tests, few issues seen

1.Need to figure out how to unreserve file manager for subsequent tests.
 Using client.plugin('fileManager', require('marionette-file-manager')); works in the first test file. In subsequent marionette tests, it
gives error  Error: fileManager - is already reserved on client

2. I have to manually create, device-storage-testing/videos folder, marionette-file-manager plugin is not able to create these folders in temp directory.

I will work with :evanxd to get these resolved and propose to update video and gallery tests to use marionette-file-manager API in a separate bug. John, wdyt?

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Mike
Please review the attached PR. Thanks

Comment 12

[Punam Dahiya [:pdahiya]]

Created bug https://bugzilla.mozilla.org/show_bug.cgi?id=943504 for updating gallery and video tests to use marionette-file-manager plugin

Comment 13

[Mike Pennisi [:jugglinmike]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Punam,

It looks like you are still working out some issues with the tests, so I'll let you finish. Feel free to re-set the review flag when you're ready for me to take a look.

Comment 14

[Punam Dahiya [:pdahiya]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Mike
I got false alarm on travis, so i was trying to debug it by adding log statement for data activity. Marionette tests are green on travis now. It's my bad, next time i will wait for travis to go through before setting review flag. Please review.

Thanks

Comment 15

[Mike Pennisi [:jugglinmike]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Punam,

Wow! This patch has come a long way since you last showed me--nice work! I've left some feedback on the GitHub pull request. Feel free to flag me for review once you've incorporated my feedback (or flag me for "needsinfo" if something doesn't make sense)

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Punam,

Thanks for reporting this bugs of file manager. I don't know if that's easy to fix. I will transfer the needinfo flag to evanxd. 

I feel we don't need to chain this video tests with file manger, if we can't use it right now. If the review is finished before the fixing of file manager, please land the code and open a follow-up issue to use file manager.

Evan, please find the issues at comment 10 and may you evaluate if it is easy to fix and do we need to wait for that??

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Punam,

Sorry, I didn't find that you already opened a followed-up bug 943504. I feel just use the testcommon from gallery app. Thanks for that.

Comment 18

[Evan Tseng [:evanxd]]

(In reply to John Hu [:johnhu] from comment #16)
> Punam,
> 
> Thanks for reporting this bugs of file manager. I don't know if that's easy
> to fix. I will transfer the needinfo flag to evanxd. 
> 
> I feel we don't need to chain this video tests with file manger, if we can't
> use it right now. If the review is finished before the fixing of file
> manager, please land the code and open a follow-up issue to use file manager.
> 
> Evan, please find the issues at comment 10 and may you evaluate if it is
> easy to fix and do we need to wait for that??

Hi John,

I already replied the issues in https://bugzilla.mozilla.org/show_bug.cgi?id=943504#c1.

Comment 19

[Evan Tseng [:evanxd]]

Hi all,

Updated the doc.

We could refer to the doc in https://github.com/mozilla-b2g/marionette-file-manager#usage to use marionette-file-manager correctly.

Comment 20

[Punam Dahiya [:pdahiya]]

Hi Mike
I have updated PR with review feedback. As for comment around share and delete tests using activity handling (https://github.com/mozilla-b2g/gaia/pull/12717#discussion_r7942903), we are using similar tests in email. 

I am open to other approaches  for better handling marionette tests around opening camera app and share activity for videos. I will needinfo Evan on the same. Please review. Thanks

Comment 21

[Punam Dahiya [:pdahiya]]

Hi Evan
I have left comment in github with questions on your suggested approach for handling tests on opening camera app and share activity for Videos. Thanks

Comment 22

[Mike Pennisi [:jugglinmike]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Punam,

This patch is looking good! Evan's idea sounds promising, and I want to give him a chance to respond to your `needsinfo` request before landing these tests. I'll just unset the flag for now, but you can request review again once we hear back from Evan.

Comment 23

[Evan Tseng [:evanxd]]

Hi Mike and Punam,

Sorry for the late reply, please check the GitHub for the comments.

Comment 24

[Evan Tseng [:evanxd]]

See it in https://github.com/mozilla-b2g/gaia/pull/12717#discussion_r8224972.

Comment 25

[Punam Dahiya [:pdahiya]]

(In reply to Evan Tseng [:evanxd] from comment #24)
> See it in https://github.com/mozilla-b2g/gaia/pull/12717#discussion_r8224972.

Thanks Evan for clarifying. I have updated PR with your suggestion in comment https://github.com/mozilla-b2g/gaia/pull/12717#discussion_r8225953.

Comment 26

[Punam Dahiya [:pdahiya]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Hi Mike,
Pull request is updated with Evan inputs. Please review. Thanks

Comment 27

[Evan Tseng [:evanxd]]

Hi Puman,

How do you think about marionette-file-manager things in https://github.com/mozilla-b2g/gaia/pull/12717/files#r8256781?

If we need to land the patch, we could file a follow up bug for it.

Thanks.

Comment 28

[Evan Tseng [:evanxd]]

Sorry, the comments is in https://github.com/mozilla-b2g/gaia/pull/12717#discussion_r8256781.

Comment 29

[Punam Dahiya [:pdahiya]]

(In reply to Evan Tseng [:evanxd] from comment #27)
> Hi Puman,
> 
> How do you think about marionette-file-manager things in
> https://github.com/mozilla-b2g/gaia/pull/12717/files#0?
> 
> If we need to land the patch, we could file a follow up bug for it.
> 
> Thanks.

Thanks Evan , Tested and video tests works with marionette-file-manager API version 0.0.2. Updated the attached pull request to use marionette-file-manager API.

Comment 30

[Punam Dahiya [:pdahiya]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Adding Evan as reviewer for marionette-file-manager changes in video tests

Comment 31

[Punam Dahiya [:pdahiya]]

looking into travis failure, please wait for travis to be green before review, thanks

Comment 32

[Mike Pennisi [:jugglinmike]]

Looks like the pull request is green again, but it's getting late here in Boston. I'll take a look at this in the morning. I'm curious, though: what went wrong?

Comment 33

[Mike Pennisi [:jugglinmike]]

Comment on attachment 814175 [details] [review]
Pull requests for video info integration test

Nice work, Punam!

Comment 34

[Punam Dahiya [:pdahiya]]

(In reply to Mike Pennisi [:jugglinmike] from comment #32)
> Looks like the pull request is green again, but it's getting late here in
> Boston. I'll take a look at this in the morning. I'm curious, though: what
> went wrong?

Thanks Mike for review. In Bug 907013 
https://github.com/mozilla-b2g/gaia/commit/a6c2af8d32274113e81d2c57e23d0621af118e53#diff-ea2f6e41c928728ac9ce1c62ff295c0aR44
ids in confirm modal dialog box are changed to class names.

We are using confirm dialog box in video delete tests, causing 
delete tests to fail.  I have added delete tests back after fixing delete tests  to use 'modal-dialog-confirm-ok' as class name. 
Will appreciate if you can review delete video test before i land the patch. Thanks

Comment 35

[Evan Tseng [:evanxd]]

Hi Punam,

We left comments for the patch in GitHub. Please request review again once we update the patch.

Comment 36

[Evan Tseng [:evanxd]]

Just a little nit, the patch looks good. Nice work! :D

Comment 37

[Punam Dahiya [:pdahiya]]

(In reply to Evan Tseng [:evanxd] from comment #35)
> Hi Punam,
> 
> We left comments for the patch in GitHub. Please request review again once
> we update the patch.

PR updated with the feedback in GitHub. Please review. Thanks

Comment 38

[Evan Tseng [:evanxd]]

Hi Punam,

Nice work!:)

Comment 39

[Mike Pennisi [:jugglinmike]]

Punam,

This was a little confusing to me because the commit you referenced did not actually modify the element's ID or classname. I think to avoid this confusion, you should also update the following comment to reference `system/js/app_modal_dialog.js`:

    /**
      * @return {Marionette.Element} ok button in confirm delete files dialog.
      */
     get confirmOk() {
       // selector specific to the out-of-app confirm dialog
       // found in system/index.html
       return this.client.findElement('button.modal-dialog-confirm-ok');
     },

With that change, this should be good to merge!

Comment 40

[Punam Dahiya [:pdahiya]]

Updated PR, waiting for travis to be green before merge

Comment 41

[Punam Dahiya [:pdahiya]]

Finally, travis is green. Patch landed to master https://github.com/mozilla-b2g/gaia/commit/7deffe21aedd0b1908d981be4a348f148a4332e8

Comment 42

[Gregor Wagner [:gwagner]]

Backed out for constant travis failures
https://github.com/mozilla-b2g/gaia/commit/8001e884e1d5d3217de57361203ccf63218b03bb

Comment 43

[Evan Tseng [:evanxd]]

Hi Gregor,

Could you provide the travis failures page here?

Comment 44

[Punam Dahiya [:pdahiya]]

The link to failing job on travis for PR #12717 https://travis-ci.org/mozilla-b2g/gaia/jobs/15541347. This failure is intermittent on travis and looks like race condition. 

I have created a new pull request by calling the click on the first thumbnail item from the test.
https://github.com/mozilla-b2g/gaia/pull/14781/files#diff-8fbee53f9fed98efa2896fc5305381edR62

Attaching the pull request to the bug for review. Thanks

Comment 45

[Punam Dahiya [:pdahiya]]

Attachment: Pull request of video tests

Comment 46

[Punam Dahiya [:pdahiya]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

Setting the review flag. Mike, Evan Can one of you please review this change to handle travis failure. The change is calling click on first thumbnail from the test directly instead of idx position thumbnail. marionette_js job is green on travis. Thanks

Comment 47

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

Hi Punam,

I'm not convinced this is the right solution. Firstly: it exposes implementation details (element names, DOM events) to the test bodies. See comment 6 for the justification around keeping the integration test bodies themselves abstract.

Also, it introduces an alternative to a suspicious method but does not remove that method (`selectThumbnail`). If that method is indeed "racy", then it shouldn't be available for others to reuse.

Most importantly, though: it's not clear how this change avoids the race condition. The backed-out patch called `selectThumbnail`, which internally called `getThumbnails` and then `Element#click`. While this version forgoes the `selectThumbnail` method, it essentially performs the same operation: it calls a new `thumbnail` getter method and then `Element#click`. Unless the race condition is somehow triggered by `Client#findElements` but *not* `Client#findElement`, then I don't see how the new approach could solve the problem.

If anything, this approach looks *more* dangerous. It skips the `waitFor` condition immediately following the `click` interaction (so it may now be possible to continue the test before the DOM has updated).

Now that we know there is a race condition in these tests, we need to be very careful about how we resolve it; simply seeing the tests pass once is not sufficient. We're going to need a more conclusive understanding of what is going wrong before we can confidently merge this into `master` again.

To start: try to understand why the value of `firstItem` was undefined in that particular test run. If I had to guess, I would say they there were *no* items available by the time the code executed. Maybe the `enterSelectionMode` method completes too early (is the application done setting up selection mode as soon as the `thumbnailSelectView` element is defined?) or (tracing even further back) maybe the `launch` method completes too early (is the application truly "ready" once the `body` element is defined?).

Comment 48

[Punam Dahiya [:pdahiya]]

(In reply to Mike Pennisi [:jugglinmike] from comment #47)
> Comment on attachment 8349471 [details] [review]
> Pull request of video tests
> 
> Hi Punam,
> 
> I'm not convinced this is the right solution. Firstly: it exposes
> implementation details (element names, DOM events) to the test bodies. See
> comment 6 for the justification around keeping the integration test bodies
> themselves abstract.
> 

Added the abstracted methods such as playVideoFromGalleryView, selectThumbnail to improve readability of the tests. 


> Also, it introduces an alternative to a suspicious method but does not
> remove that method (`selectThumbnail`). If that method is indeed "racy",
> then it shouldn't be available for others to reuse.
> 
> Most importantly, though: it's not clear how this change avoids the race
> condition. The backed-out patch called `selectThumbnail`, which internally
> called `getThumbnails` and then `Element#click`. While this version forgoes
> the `selectThumbnail` method, it essentially performs the same operation: it
> calls a new `thumbnail` getter method and then `Element#click`. Unless the
> race condition is somehow triggered by `Client#findElements` but *not*
> `Client#findElement`, then I don't see how the new approach could solve the
> problem.
> 
> If anything, this approach looks *more* dangerous. It skips the `waitFor`
> condition immediately following the `click` interaction (so it may now be
> possible to continue the test before the DOM has updated).
> 
> Now that we know there is a race condition in these tests, we need to be
> very careful about how we resolve it; simply seeing the tests pass once is
> not sufficient. We're going to need a more conclusive understanding of what
> is going wrong before we can confidently merge this into `master` again.
> 

selectThumbnail method is updated to check for first thumbnail item to be successfully loaded before calling the click event on that thumbnail. Additionally, i have added explicit "waits" after each user interaction in all test cases.

> To start: try to understand why the value of `firstItem` was undefined in
> that particular test run. If I had to guess, I would say they there were
> *no* items available by the time the code executed. Maybe the
> `enterSelectionMode` method completes too early (is the application done
> setting up selection mode as soon as the `thumbnailSelectView` element is
> defined?) 

Thanks for pointing it, this could be main reason of undefined failure on travis that is thumbnailSelectView is displayed before the thumbnail item is loaded. 

or (tracing even further back) maybe the `launch` method completes
> too early (is the application truly "ready" once the `body` element is
> defined?).

It should not be launch method completes too early as the enterSelectionMode in Share test completes successfully and the undefined failure error occurs inside subsequent selectThumbnail method


I have updated PR to handle explicit waits in all tests.  Please feel free to suggest if you still see some areas where intermittent failures can originate on travis.  Thanks

Comment 49

[Mike Pennisi [:jugglinmike]]

Attachment: 14781.patch

Comment 50

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

Hi Punam,

The TravisCI build for this pull request is currently failing. I'm unable to run your tests locally at the moment, so I'm not sure if this is a deterministic failure or not. If you need help debugging, I will be more available starting January 6 (though I may be able to check in again before then). Here's the error report:

    41 pending
    1 failing

    1) played video should play video and share:

    AssertionError: the activity name should be share.
      at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/video/test/marionette/video_play_test.js:60:12)
      at Test.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:211:32)
      at Runner.runTest (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:372:10)
      at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:448:12
      at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:297:14)
      at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:307:7
      at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:245:23)
      at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7
      at Hook.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:213:5)
      at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:257:10)
      at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7
      at done (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:185:5)
      at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:197:9
      at Object.executeHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)
      at process._tickCallback (node.js:415:13)

Comment 51

[Punam Dahiya [:pdahiya]]

Hi Mike
I have updated PR by removing video tests that were failing intermittent on travis. Also, I have run video marionette tests multiple times on travis to
take out orange tests. Attached PR passed on travis when tried several times 
https://travis-ci.org/mozilla-b2g/gaia/builds/16301956
https://travis-ci.org/mozilla-b2g/gaia/builds/16305385

Please review
Thanks

Comment 52

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

It looks like all the race conditions are resolved or removed--nice work!

I see that you've avoided one race condition by adding a `waitForElement` invocation to the beginning of the "check video lists" test. Given that, I have one more request:

1. Add a `waitForElement` call to the end of the `setup` function in `video_list_test.js`.
2. Remove the `waitForElement` calls that begin the "check video lists" and "open camera app" tests.

Here's why: the `waitForElement` condition is actually part of the `setup` for *any* test in `video_list_test.js` (it's even encapsulated in the `enterSelectionMode` method that begins the "share video" test). Moving it is more than just an attempt to save keystrokes, though: other developers that want to add tests in the future would not necessarily know that they had to explicitly `waitForElement` before each new test. When you encapsulate this logic in the `setup` function, every test (current and future) will be guaranteed to be in a "safe" state the moment it begins to execute.

Does this make sense?

Comment 53

[Punam Dahiya [:pdahiya]]

Thanks Mike for your inputs. I have updated the PR  and travis is green on trying multiple run. 
https://travis-ci.org/mozilla-b2g/gaia/builds/16488087
Please review.

Comment 54

[Evan Tseng [:evanxd]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

Mike already helped for the review.
I could do other review.
Thanks, Mike.

Comment 55

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8349471 [details] [review]
Pull request of video tests

Looks great, Punam! There's a minor conflict in the package.json file (see below)--it looks like we just need to keep the line introducing the "execSync" dependency but append a comma to it. Feel free to merge once that's resolved!

    diff --cc package.json
    index 7cc20fc,3c48e04..0000000
    --- a/package.json
    +++ b/package.json
    @@@ -34,6 -34,6 +34,10 @@@
          "grunt": "0.4.2",
          "grunt-contrib-clean": "0.5.0",
          "grunt-jsdoc": "0.5.0",
    ++<<<<<<< HEAD
     +    "execSync": "~1.0.0"
    ++=======
    +     "marionette-file-manager": "0.0.2"
    ++>>>>>>> Bug924170 - [Video] - Add Marionette tests for Video App r=jugglinmike
        }
      }

Comment 56

[Punam Dahiya [:pdahiya]]

Hi Mike
Thanks for review. Merge conflict was due to a commit from PR https://github.com/mozilla-b2g/gaia/pull/15004 which got reverted. Latest master doesn't show the conflict. I have successfully merged the patch to master. Thanks

Comment 57

[Punam Dahiya [:pdahiya]]

Patch merged to master https://github.com/mozilla-b2g/gaia/commit/df4b0b56f41d716aa9ac68da828e02b8ff258c52