WebM videos can be saved as PDF

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: vivek, Mentored)

Tracking

unspecified
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
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).
Assignee: nobody → vivekb.balakrishnan
Mentor: michael.l.comella
(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.
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8465056 [details] [diff] [review]
1034261_mock.patch

Patch that tests saveaspdf button is disabled for vidoe mime time.
Video playback mocked with content:locationChange event.
Attachment #8465056 - Flags: review?(michael.l.comella)
(Assignee)

Comment 5

4 years ago
Created attachment 8465058 [details] [diff] [review]
1034261_mock.patch

nit corrected : unnecessary import removed
Attachment #8465056 - Attachment is obsolete: true
Attachment #8465056 - Flags: review?(michael.l.comella)
(Assignee)

Updated

4 years ago
Depends on: 996227
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
Attachment #8465058 - Flags: feedback+
Comment on attachment 8465058 [details] [diff] [review]
1034261_mock.patch

Finkle, what do you think of this approach?
Attachment #8465058 - Flags: feedback?(mark.finkle)
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.
(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).
(Assignee)

Comment 10

4 years ago
Created attachment 8466458 [details] [diff] [review]
1034261_mock_v2.patch

A new patch that solves the timing issue with the earlier patch.
Attachment #8465058 - Attachment is obsolete: true
Attachment #8465058 - Flags: feedback?(mark.finkle)
Attachment #8466458 - Flags: feedback?(michael.l.comella)
(Assignee)

Comment 11

4 years ago
Adding the cleared feedback flag for Finkle
Flags: needinfo?(mark.finkle)
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.
Attachment #8466458 - Flags: feedback?(michael.l.comella) → feedback+
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.
Flags: needinfo?(mark.finkle)
(Assignee)

Updated

4 years ago
Depends on: 1049003
(Assignee)

Comment 14

4 years ago
Created attachment 8468844 [details] [diff] [review]
1034261_mock.patch

A new patch with sleep call.
Attachment #8466458 - Attachment is obsolete: true
Attachment #8468844 - Flags: review?(michael.l.comella)
(Assignee)

Comment 15

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

This patch doesn't use sleep call. Depends on #1049003 to land.
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.
Attachment #8468844 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 17

4 years ago
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
(Assignee)

Comment 18

4 years ago
Created attachment 8473793 [details] [diff] [review]
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
Attachment #8468844 - Attachment is obsolete: true
Attachment #8473793 - Flags: review?(michael.l.comella)
Comment on attachment 8473793 [details] [diff] [review]
1034261_v2.patch

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

Cool beans. Thanks, vivek!
Attachment #8473793 - Flags: review?(michael.l.comella) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/20fb108b2351
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.