1034261 - WebM videos can be saved as PDF

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

1) Open a webm video such as http://video.webmfiles.org/big-buck-bunny_trailer.webm
2) Open options -> Page -> Save as pdf

Expected: Save as PDF is a disabled option
Actual: Save as PDF is enabled

Comment 1

[Wesley Johnston (:wesj)]

I wonder if this is a valid way to get a screenshot of a video (especially since we lack desktops ability to take screenshots of video right now).

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> I wonder if this is a valid way to get a screenshot of a video (especially
> since we lack desktops ability to take screenshots of video right now).

Note that it's possible to take screenshots via monitor, or on newer devices, the built-in screenshot mechanics.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re our conversation on IRC, I don't believe logcat statements are dumped to the test output, but if you run logcat independently, the logs should be interwoven.

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: 1034261_mock.patch

Patch that tests saveaspdf button is disabled for vidoe mime time.
Video playback mocked with content:locationChange event.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 1034261_mock.patch

nit corrected : unnecessary import removed

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8465058 [details] [diff] [review]
1034261_mock.patch

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

Were you able to get this working without failures and missing assertions?

::: mobile/android/base/tests/testAppMenuPathways.java
@@ +32,5 @@
>          // Test save as pdf functionality.
>          // The following call doesn't wait for the resulting pdf but checks that no exception are thrown.
>          mAppMenu.pressMenuItem(AppMenuComponent.PageMenuItem.SAVE_AS_PDF);
> +
> +        // Generate a mock Content:LocationChange message with video mime-type for current tab with tabId 0.

You should add a comment, probably at the bottom of the method, that mentions that this method changes Java state, but not the associated JS state, so to clear the test state, a new url should be loaded (which will trigger Content:LocationChange).

@@ +35,5 @@
> +
> +        // Generate a mock Content:LocationChange message with video mime-type for current tab with tabId 0.
> +        JSONObject message = null;
> +        try {
> +            message = new JSONObject("{'contentType':'video/webm'," +

nit: I would prefer if this object was constructed via .put* - it provides type checking (e.g. tabID), and prevents json syntax errors.

@@ +46,5 @@
> +        } catch (Exception ex) {
> +            mAsserter.ok(false, "exception in testSaveAsPDFPathway", ex.toString());
> +        }
> +
> +        // Mock the video playback with generated message and Content:LocationChange event.

nit: -> "Mock video playback" ... "with the generated"

[1]: https://en.wikipedia.org/wiki/Mock_object

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8465058 [details] [diff] [review]
1034261_mock.patch

Finkle, what do you think of this approach?

Comment 8

[Wesley Johnston (:wesj)]

I was thinking the other day, it might be neat if we changed this to just a "Save video" item when you're on a video page.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Wesley Johnston (:wesj) from comment #8)
> I was thinking the other day, it might be neat if we changed this to just a
> "Save video" item when you're on a video page.

Reminds me of bug 1042726, where a user wanted to save a text file.

Though, if we can save a video, we should be able to save any type of page (bug 924751).

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: 1034261_mock_v2.patch

A new patch that solves the timing issue with the earlier patch.

Comment 11

[Vivek Balakrishnan[:vivek]]

Adding the cleared feedback flag for Finkle

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8466458 [details] [diff] [review]
1034261_mock_v2.patch

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

Looking pretty good - is there anything left to do after addressing my comment?

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +112,5 @@
>                  // Parent menu "page" is enabled, open page menu and check for menu item represented by pageMenuItem.
>                  pressMenuItem(MenuItem.PAGE.getString(mSolo));
>  
> +                // Wait a sec for the menu to update.
> +                mSolo.sleep(1000);

As discussed on IRC, this is probably caused by attempting to find the Android menu views before the submenu has been fully opened. Thus, there is probably a better way to do this via searching for the open menu's text.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I took a quick look at the patch. It seems to take the "mock" approach I was thinking about. Looks good to me, except for the mSolo.sleep(1000) but Michael and Vivek are working through that issue.

Comment 14

[Vivek Balakrishnan[:vivek]]

Attachment: 1034261_mock.patch

A new patch with sleep call.

Comment 15

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8468844 [details] [diff] [review]
1034261_mock.patch

This patch doesn't use sleep call. Depends on #1049003 to land.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8468844 [details] [diff] [review]
1034261_mock.patch

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

lgtm if you have a try run to show that this patch, and its dependent patch, are green.

Comment 17

[Vivek Balakrishnan[:vivek]]

Try server logs:
https://tbpl.mozilla.org/?tree=Try&rev=788c07943ae1

The tested patch contains changes related to bug 1049003 and its dependent bug 1034261

Comment 18

[Vivek Balakrishnan[:vivek]]

Attachment: 1034261_v2.patch

A new patch with mock video test done before the save as pdf test. 

Try server log for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=9ce98d67e701

The tests were swapped because it was found that save as pdf functionality is slow and causes the mock video test to fail.

A patch with a sleep call between the tests before swapping was tested in try and the logs are here :
https://tbpl.mozilla.org/?tree=Try&rev=f37ff1d1ad25

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8473793 [details] [diff] [review]
1034261_v2.patch

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

Cool beans. Thanks, vivek!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/20fb108b2351

I tweaked your commit message before pushing. In general, your commit message should be summarizing what the patch is doing, not restating the problem it's solving. Also, it's preferred that you use your full name in your hg committer information if possible. Thanks!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20fb108b2351