960813 - [B2G][Browser][RTSP] Unable to add RTSP bookmark to the "Home screen"

Status: RESOLVED FIXED

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

Description

[sarsenyev]

Attachment: videobug2.mp4

Description:
When adding a bookmark with RTSP link, the bookmark is not added

Repro Steps:
1) Updated Buri to BuildID: 20140115004003
2) Launch the browser visit Mozilla website
3) Tap "Star" icon to add it to bookmark
4) Input a RTSP link and save it
5) Tap "Add to homescreen"

Actual:
The bookmark is not added, the screen with option closes

Expected:
The bookmark is added to the home screen

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140115004003
Gaia: 14e199d6a9ad917eacad883820a9f7619dbf42c8
Gecko: d7260b206e91
Version: 28.0a2
Firmware Version: 1.2-device.cfg

Notes:
Repro frequency: 100%
https://moztrap.mozilla.org/manage/cases/?filter-id=11041
See attached: video file

Comment 1

[Jason Smith [:jsmith]]

The STR is very unclear here - it's intermixing discussion of bookmarking vs. adding to the homescreen. What's the bug focusing on here?

Comment 2

[sarsenyev]

When RTSP stream opens, there is no option (star) to add to home screen, the option is available when this stream is added as bookmark first, then menu with options are available "add to homescreen or "unbookmark" , so when a user chooses add to homescreen, nothing happens
 Scenario is when the user doesn't want anymore to open browser to see the previously saved RTSP link as bookmark, so he is decided to add it to the homesceen

Comment 3

[Jason Smith [:jsmith]]

Sounds like a broken experience then on a 1.3 committed feature.

Comment 4

[Gregor Wagner [:gwagner]]

Ben, Dale any idea whats going on here?

Comment 5

[Dale Harvey (:daleharvey)]

The particular steps to reproduce are broken, however going to 

http://www.webmfiles.org/demo-files/

and attempting to save one of the video files to the homescreen does break

Comment 6

[Jason Smith [:jsmith]]

(In reply to Dale Harvey (:daleharvey) from comment #5)
> The particular steps to reproduce are broken, however going to 
> 
> http://www.webmfiles.org/demo-files/
> 
> and attempting to save one of the video files to the homescreen does break

I have a test site here - http://mozilla.github.io/qa-testcase-data/Video/youtubeRTSPURLSearch.html btw you could use for testing this. Just search for something & you'll get RTSP videos for the top 50 search results from YouTube.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846

WIP patch for feedback.

* allows user to bookmark anything that's a valid URL
* however, allowing non-URLs so need to figure that out ;)

Comment 8

[Dietrich Ayala (:dietrich)]

* add tests for saving URLs with non http/https schemes as bookmarks to homescreen

* add test for bookmarking non-URLs

Comment 9

[Dietrich Ayala (:dietrich)]

Latest version contains:

* enables saving any valid URL as a homescreen bookmark (now matches browser bookmark behaviour)

* adds live validation of URL at browser/homescreen bookmark creation time

* adds live validation of URL when editing a browser bookmark

Need to add tests per comment #8

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8368198 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846

Added some comments, theres some existing code to work around that doesnt make a ton of sense, but the bookmark fixes look good

For WIP patches dont we f? instead of r?, clearing though

Comment 11

[Dietrich Ayala (:dietrich)]

PR now has more tests.

I added some browser tests that should be failing (but are passing), and the pre-existing homescreen bookmark tests don't actually save a bookmark (and can't!).

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 8368198 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846

Some more cleanup, and found a way to unit test the UI part at least.

Comment 13

[Dale Harvey (:daleharvey)]

Could you clarify the STR that you are solving here? there is quite a few quirks around browser bookmarking / homescreen and want to know what you are going for in terms of use case + expected behaviour

Comment 14

[Dietrich Ayala (:dietrich)]

Yeah I've gone through the bookmarking code quite a bit now, definitely could be made simpler.

But for now, the substantive changes in this patch:

* Updates all bookmark add/edit UI to check that the URL is valid.

* Those forms now only allow bookmark to be saved if the URL is valid.

* Updated the bookmark-to-homescreen activity to allow any valid URL, instead of just HTTP(S).

* Added a regression test to browser db tests to ensure non-HTTP(S) URLs are saved.


Other minor changes in this patch, to pre-existing code:

* Noted a couple of places where we silently fail. Can add proper roundtrip error handling later.

* Logged an unhandled failure. Can add proper roundtrip error handling later.

* Moved a few files out of xfail.list, along with the changes needed to pass jshint.

* Added a couple of stub tests to browser db tests which are currently passing but should fail (empty title, invalid URL).

Comment 15

[Dietrich Ayala (:dietrich)]

Also, I just found a Homescreen integration test which is currently commented out. I'm going to try and get it running, but we don't need to hold off on landing this blocker fix on that.

Comment 16

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 17

[Dietrich Ayala (:dietrich)]

Patch needs review, then can land.

Comment 18

[Dale Harvey (:daleharvey)]

I can see whats changed, but its the steps to reproduce I need, like in terms of opening the browser what do you do for a santity test (or tests), its also gonna help QA when it comes to verifying it.

Things like 'valid url' are fuzzy terms and on testing this patch I am seeing a lot of behaviour that I might expect this patch to change to not effect anything, (also clarifying between homescreen and browser when you mention bookmarks would be handy)

Cheers

Comment 19

[Dietrich Ayala (:dietrich)]

STR: Save any non-HTTP(S) URL as a bookmark to Homescreen.

Valid URL shouldn't be fuzzy, is defined here: http://mxr.mozilla.org/gaia/source/shared/js/url_helper.js#18

Re homescreen vs browser bookmark - this patch results in URLs being treated consistently, regardless of homescreen and browser (those URLs sourced from same place - browser!).

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to Dietrich Ayala (:dietrich) from comment #15)
> Also, I just found a Homescreen integration test which is currently
> commented out. I'm going to try and get it running, but we don't need to
> hold off on landing this blocker fix on that.

bug 952610

Comment 21

[Dale Harvey (:daleharvey)]

Sorry but save url as homescreen isnt steps to reproduce

"Open browser, enter hddp://fakedomain then save to homescreen, see that this invalid url can be saved as a bookmark"

Something that the bug reporter, qa, the dev and the reviewer can follow exactly, reproduce and use to agree whether its a bug and whether its been fixed

Whether a url is valid or not is very fuzzy, we have bugs open against our implementation of html5 url validation, there are open questions about whether we should load app:// url or various other schemes.

The reason I wanted the STR as my one ^ failed silently in both master and your patch, which I would consider a bug, but it may not be the bug you were fixing

Comment 22

[Dietrich Ayala (:dietrich)]

Dale, comment #0 has detailed STR too ;)

Thanks for the information about that URL failing silently! I have that in the unit tests, which passed... so I'll hit those again to see what's up.

Comment 23

[Dietrich Ayala (:dietrich)]

(In reply to Dale Harvey (:daleharvey) from comment #21)
> Whether a url is valid or not is very fuzzy, we have bugs open against our
> implementation of html5 url validation, there are open questions about
> whether we should load app:// url or various other schemes.

Hehe, so our operating system might not have decided what a URL is (bizaarro), but we don't have to solve that in this patch.

This patch just needs to be able to use the *currently-used definition* of a valid URL as a homescreen bookmark, instead of restricting to HTTP(S) only.

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 8368198 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846

Looks good, thanks, and sorry for the delay

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3993618b1a34

Comment 26

[Jason Smith [:jsmith]]

https://github.com/mozilla-b2g/gaia/commit/9d7ec082e7e7c7db879a8b1d98ea5dd4cd3155bb

Comment 27

[Dietrich Ayala (:dietrich)]

(In reply to Dale Harvey (:daleharvey) from comment #21)
> The reason I wanted the STR as my one ^ failed silently in both master and
> your patch, which I would consider a bug, but it may not be the bug you were
> fixing

This problem occured with or without this patch, with error:

JavaScript error: http://browser.gaiamobile.org:8080/js/browser_db.js, line 221: NS_ERROR_DOM_BAD_URI: Access to restricted URI denied

I suspected that maybe the neterror chrome URL was what was being passed.

HOWEVER...

In the latest build, this no longer occurs because the new browser error pages are modal! You have a choice of close or try again, so can never get to the point at which you can bookmark an invalid URL.

The STR of comment #0 of this bug are still valid however, since you can change the URL when creating a homescreen bookmark. So this bug is still present, requiring this patch.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x  9d7ec082e7e7c7db879a8b1d98ea5dd4cd3155bb
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 29

[Ryan VanderMeulen [:RyanVM]]

1.3 blockers no longer have auto-approval to land. Please request gaia-v1.3 approval on the patch for uplift.

Comment 30

[Dietrich Ayala (:dietrich)]

I heard RTSP is no longer shipping in 1.3. If that's the case, this bug is no longer needed on the branch.

Who can confirm this is the case? Kevin, do you know?

Comment 31

[Kai-Chih Hu [:khu]]

Wesley, can you help? Thanks.

Comment 32

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

To clarify, RTSP in 1.3 still supports audio streaming. 
It's the video streaming being disabled.

Comment 33

[Dietrich Ayala (:dietrich)]

Ok, thanks Wesley. Rebased patch coming up!

Comment 34

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16281

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
I'm not sure why this was considered a blocker - do users bookmark RTSP URLs?

[Bug caused by] (feature/regressing bug #): None, RTSP is new in 1.3.

[User impact] if declined: User cannot bookmark RTSP URLs.

[Testing completed]: See unit tests in patch. Also added validation in the UI.

[Risk to taking this patch] (and alternatives if risky): Low risk.

[String changes made]: None.

Comment 35

[Jason Smith [:jsmith]]

(In reply to Dietrich Ayala (:dietrich) from comment #34)
> Created attachment 8376092 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16281
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> I'm not sure why this was considered a blocker - do users bookmark RTSP URLs?

Now that I think about this more - I think you are right. The use case here isn't realistic because:

The only way you could bookmark a RTSP URL right now is by providing the direct URL during the add bookmark screen. It's not possible ever bookmark the RTSP URL through the star button ever, as don't render RTSP media content in the browser - it renders in the video app. The average joe user probably isn't going through the painful effort of trying to type out an exact RTSP URL into the add bookmark UI, as it's too much effort. Given those factors, I think we pull this blocking list.

Sorry I didn't think about this earlier. I realize now that this doesn't really make sense to take to 1.3.

Comment 36

[[:fabrice] Fabrice Desré]

Comment on attachment 8376092 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16281

clearing since it's not wanted on 1.3 anymore